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
$id
updates #1537$id
updates #1537Changes from 16 commits
71dd816
16c475a
64fbf86
a38117a
e9aed4a
1d61c87
4392b25
43c8889
39c0e18
43c260c
878517d
636aa37
8407415
cd52a3e
9a8ae14
5184c29
4d6fdb5
7f04f62
b64d297
File 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 find this paragraph confusing. The word "intervening" doesn't make sense in this context. I also found it awkward that it starts by saying it will behave correctly, then says it must not be relied upon. I think it would be more clear if it was the other way around.
I think this was originally about unknown keywords. Since unknown keywords aren't allowed anymore, it becomes about custom vocabulary defined keywords. Therefore, I think this is saying that you can't use
$id
in a schema in a custom keyword. I don't think that's what we want.$id
should work in any known schema, even in custom keywords.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 paragraph was just moved up. It was previously at line 1404 (which you can see just below).
The text is "will behave correctly under implementations that attempt to use any reference target as a schema" which is correct.
It's not just about unknown keywords. It's also about keywords that can define structure but not subschemas, like
default
orconst
(or custom keywords with structured data). The point of this paragraph is to say that$id
in any place where a schema is not expected is to treated as data, not an identifier.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 noticed that it was moved. I'm not arguing about its correctness. I can't even say anything about its correctness at this point because I don't understand what it's trying to say.
I don't see how that paragraph can be understood to be about that. It appears this only applies to custom keywords with identical semantics to
$defs
.That's well stated and clear. I suggest replacing the paragraph with some variation of 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.
Yeah, reading it again, I don't think it's necessary. It looks like it's just trying to explain that you can't have an
$id
between the keyword and a potential subschema that isn't a subschema because of the structure of the keyword. I'm not sure that needs explaining, so I'll just remove it.I'm not sure if we need to keep the "requirement" about "MUST NOT be relied upon". It's definitely a requirement on schema authors, but it's a weird one at that.
The next paragraph actually covers exactly what I was saying:
I still think this can be cleaned up, though.