Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added migration page overview #1061
base: main
Are you sure you want to change the base?
Added migration page overview #1061
Changes from 28 commits
859a6f193bcabad4c8b248eb972175ee2a1a967f66b22b2ffc5d352e136ee2efa6391b83536f1dd364badd1d4e733b69ab9c583ce5a4b8b324488f94e5a08dd17e308c5b1f8795276a0288e5d971992928cc36f045aecbc65ba8989891c6e5bdfc653ef35dea68d4f365325b8419ea3643f9816bb62eaa757f1f95eae10093da171994f731ffcf639b43946885b9ceb81ebeead718e513a5a63233e536b266963be75e0da78d912a5a46c9d91c3b63ddec102f0fd4c2File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we should improve on the description of the change. Instead of just saying it partially replaces
$id, we can mention something like the fact that$ids that defined anchor-only URI, like$id: #fooare now replaced with$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.
My assumption is that this needs to be very brief to not explode the table. I'm not sure we have the space to explain the change properly. My understanding was that this kind of thing was going to be covered in detail elsewhere.
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.
Hi @jviotti @kwennB , I think because
$dynamicRef,$dynamicAnchoradded more flexibility / usage over $recursiveAnchor.Maybe we can say it as replacement or enhancement ? OR something like 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.
This entry feels weird. We are saying that
$refwas removed on2019-09and replaced by$ref, which feels very confusing. The change this specific entry should be documenting is that before 2019-09,$reftook precedence over any other sibling keyword. In other words, before 2019-09, if you put any other keywords alongside$ref, it would be ignored.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.
Do we need a description here? I feel we would be just repeating the description of the entry above?
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.
If describing the change in more detail is an option, we wouldn't need two entries.
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.
In 2019-09, this keyword was modified to produce annotations
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 think we need to note that things started producing annotations in 2019-09. Nothing produced annotations before 2019-09. I think it's given or at least can be generally stated once at the top of the table that annotations only became a thing in 2019-09.
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
containskeyword was slightly changed in 2020-12. In 2020-12, it now emits annotations and affectsunevaluatedItemsThere 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 think these
contentEncodingentries make sense.contentEncodingas a keyword remains even up to 2020-12There 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.
@jviotti, see #1061 (comment). This keyword has a really complicated history. It does exist today, but there was a period when it didn't because it was moved into the
mediakeyword. I suggested two entries because there wasn't a way to express that gap.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 keywords in this table are order alphabetically, so
mediahere doesn't make sense?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.
Also, I don't think we should document Hyper-Schema stuff here. The overall Hyper-Schema specification is on pause, so I suggest ignoring it for now
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 only suggested including
mediain order to tell the complicated story ofcontentEncoding. In the beginning, there was only one spec for core, validation, and hyper-schema. I think it would make sense if we considered the originalcontentEncodingpart of hyper-schema even though it wasn't a separate spec yet. Then we can remove these two entries and pretend it was introduced in draft-07 even though it's a little more complicated than 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.
I don't think the notes make sense.
contentEncodingnever replaced any other keyword. That said, on 2019-09, it became an annotation keywordThere 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.
Same here. I don't think we should bother with Hyper-Schema, and this keyword also became an annotation in 2019-09
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.
By understanding, the specification covers - Core and Validation.
The hyperschema is a hypermedia vocabulary—I don't think we should have it here; otherwise, we may have to include other hyperschema keywords not considered for dialect migration.
cc: @jdesrosiers
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.
@jdesrosiers I am still waiting to hear from you. Thank you
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.
Sorry for the slow response. I've been away almost all of last week and I'm catching up now.
There are three specifications: Core, Validation, and Hyper-Schema. Each spec defines one or more vocabularies.
So, Hyper-Schema is a specification that defines the hyper-schema vocabulary. I was referring to "Hyper-Schema" the spec.
I addressed that in a previous comment. I suggested including it because it was previously moved from Hyper-Schema to Validation and it didn't make sense to say it replaced the old Hyper-Schema keyword without also listing the keyword it replaced. The comment I linked suggests an alternative.
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.
In 2019-09, this keyword became an annotation
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.
In 2019-09, this keyword now produces an annotation
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 keyword was turned into an annotation keyword in 2019-09. Also there were many changes in the set of permitted values this keyword could take over varios versions of the specifications. Not sure if we want to document those here
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.
Also, it started producing annotations in 2019-09
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.
itemsArray is replaced byprefixItems, butitemsObject isn't so maybe say partially replaced..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.
Technically, the object form of
itemswas removed entirely andadditionalItemswas renameditems. So, it was partially replaced and partially removed. I don't know how detailed we need to be about that in this table. It's just an overview.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.
In 2019-09, this keyword started emitting annotations
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.
In 2019-09, this keyword started emitting annotations
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.
In 2019-09, this keyword started emitting annotations
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 also weird. I think it's confusing to say
typewas replaced bytype. I think the main change you want to document here is that previously,typecould be set to a schemaThere 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
anytype was previously valid too, and now it's not. Maybe worth adding a noteThere 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.
@jviotti How best would you say to represent
type?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.
To be honest, I'm not sure. Maybe this is OK, and we can just clarify that
typeno longer takesanyand subschemas in its array form on the notes?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 this depends on how much we can put in the "Changed" column. If a brief description of the change can fit in that column, we don't need two entries. The reason I suggested two entries was because I didn't think that was an option. So, I recommend either a description of the changes in "Changes" or two entries, but not both.
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.
In 2019-09, this keyword started emitting annotations