-
Notifications
You must be signed in to change notification settings - Fork 9.2k
3.0.4: port the undisputed parts of #2140 #4062
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
While the sentence that was delete here is a bit confusing, I think there is something lost if we don't call out the difference in the meaning of "default" here. Would something like this be okay?
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 preceding sentence says
That is a MUST, not sending a value is not an allowed option.
The Schema Object's
default
talks about whatReferencing something that does not apply is confusing.
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 are referencing something to explain that, while it looks very similar -- same property name
default
in a very similar context -- it is in fact different and does not apply. I think that will be helpful to users of the spec, no?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.
@darrelmiller suggested to remove this sentence in #2140 (comment).
I agree because I find it confusing 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.
I think it would be better to change the sentence to something like:
Note that this behavior is different from the Schema Object's
default
keyword, which documents the default behavior rather than inserting the value into the data.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.
That sounds good to me. However I don't really have a problem with Ralf's PR as is.
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.
Note that keeping this sentence here is the option that was least favored by both @darrelmiller and the PR author @tedepstein, see his comment.
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.
@ralfhandl I'm certainly not proposing keeping the current sentence, which I also find confusing. But people find
default
endlessly confusing. They are particularly likely to think that JSON Schema'sdefault
means that the value ofdefault
should be written into the instance, which is not how it works at all.So the fact that we have a different
default
that does work that way is a very, very likely point of confusion. The text that was there before did not really help. However, I think the text that I proposed explains the real difference succinctly, and will keep people who read thisdefault
from thinking that JSON Schema'sdefault
works similarly. Which was always one of the endless recurring questions/complaints in the JSON Schema project, so it's a very practical and real concern, with years of experience answering this exact question to back it up.I think my experience here is a bit deeper than was available during the previous conversation.
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.
@handrews Good point, have added your proposed sentence to the end of the table cell's text, and used "receiver's behavior" instead of "default behavior" to stress the difference.