-
-
Notifications
You must be signed in to change notification settings - Fork 213
Draft 2020-12 release notes #392
Draft 2020-12 release notes #392
Conversation
✔️ Deploy Preview for condescending-hopper-c3ed30 ready! 🔨 Explore the source changes: dc25d41 🔍 Inspect the deploy log: https://app.netlify.com/sites/condescending-hopper-c3ed30/deploys/60ba68c2f151a400071b69d0 😎 Browse the preview: https://deploy-preview-392--condescending-hopper-c3ed30.netlify.app/draft/2020-12/release-notes |
The section on the changes to the |
I added the section on |
You can think of the old `$recursiveAnchor` as working the same way except that | ||
it only allowed you to create an anchor at the root of the schema and the anchor | ||
name is always empty. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another side effect of using named anchors is that you can have more than one in the same schema (and perhaps overlapping). The tree example could be extended to allow for two different types of branches, each of which is recursive and each references the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. That's worth mentioning, but I'd rather keep the example simple. I don't what this to become a tutorial and every time I attempted to give a more complex example, it kept snowballing in that direction. The link to this page is in a section called "Migrating from older drafts", so I think it's reasonable to limit the examples to how to convert a 2019-09 schema to a 2020-12 schema.
Co-authored-by: Karen Etheridge <[email protected]>
I added the section on vocabulary changes moving "unevaluated" and "format-assertion" to separate vocabs. |
I added the section on how |
I added the section on Compound Schema Documents and Bundling. |
I added an intro and filled in the remaining smaller sections. That's everything! I've removed the "draft" designation from the PR. This is ready for full review. Here's the direct preview link, https://deploy-preview-392--condescending-hopper-c3ed30.netlify.app/draft/2020-12/release-notes.html |
non-fragment-only URI-Reference, the schema the URI-Reference resolves to is | ||
used as the starting point for dynamic resolution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the schema the URI-Reference resolves to is used as the starting point for dynamic resolution" -- I don't think this is right:
If the initially resolved starting point URI includes a fragment that was created by the "$dynamicAnchor" keyword, ... Otherwise, its behavior is identical to "$ref", and no runtime resolution is needed.
(https://json-schema.org/draft/2020-12/json-schema-core.html#rfc.section.8.2.3.2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the problem. What about these two statements are in conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release notes seem to be saying that "$dynamicRef": "someuri#/foo/bar/baz"
is a valid starting point for dynamic resolution, but in fact URIs of that form can only be treated as if $ref
was used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the disconnect might be in the meaning of "dynamic resolution". The way I've used it, it includes the processes of determining whether the fragment is a usable dynamic anchor. If it isn't, the result of dynamic resolution is to do nothing and it's effective behavior is that of $ref
(just the initial resolution). Maybe you are considering "dynamic resolution" to mean only be the process after determining that we have a valid dynamic anchor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I read this as saying that if the target of the initial URI has a $dynamicAnchor present, then the dynamic scope resolution process can continue.
Maybe that entire final sentence can be removed. Implementers need to read the spec fully to understand how to implement this keyword.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would work if we remove the sentence, but I think it's better if we can provide some brief explanation of what a non-fragment-only URI-Reference would do after telling people that it's allowed. I'd rather try to improve that sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this document the place for that? (e.g. https://github.com/json-schema-org/json-schema-org.github.io/pull/392/files/dc25d4126cd3f54c770480da5aff584476fc68f8#r643340334)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're just talking about a brief note about what has changed and generally what to expect from that change. That's exactly what this document is for. The comment you reference is about expanding the example to showcase the new functionality. I wouldn't go that far in this case either.
this was unspecified and implementations may or may not support this unicode in | ||
regular expressions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong (as came up in a JSON-Schema-Test-Suite PR recently) -- unicode support is still only recommended and not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I was supposed to fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is up, #408
I'm going to create this as a draft and push up sections as I finish them. Any feedback on the draft is appreciated. No need to wait until it's done.