-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
specify if, then, and else together using annotations as interaction mechanism #1451
Conversation
Three of these keywords work together to implement conditional application of a | ||
subschema based on the outcome of another subschema. The fourth is a shortcut | ||
for a specific conditional case. | ||
These keywords contain subschemas which are conditionally applied based on the |
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 suppose this is a perfect example to me of #1445 (comment) (I didn't look at this PR first, promise :)
But to me, this makes things way less clear than what was here I think.
Just my opinion of course.
(EDIT: To be clearer on the review -- I'm -0 on the change, but if you choose to stick with the language from #1445 that I don't like, feel free to ignore the -0 and consider me OK with this regardless.)
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 a comment on this line or the concept of the PR as a whole?
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 as a whole, sorry (for being unclear).
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 purpose of this change is actually to make the interaction of if
with then
and else
more consistent.
All other keyword interactions (except contains
/minContains
, which I'm planning a PR for) are described using annotations as the communication mechanism. Implementations aren't required to actually do it that way, but it serves as a good description. That's what #1445 is about.
As they're currently defined, then
and else
depend on the evaluation result of if
. Depending on an evaluation result of a sibling keyword doesn't exist anywhere else in the spec, and there's not a defined communication mechanism to describe that behavior. But there is such a mechanism using annotations. So redefining then
and else
to depend on the annotation result of if
(which is the evaluation result) makes 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.
To me -- again just me -- it is undoubtedly going to be clear to current readers of the spec what the intended behavior is, regardless of formality. It's the way any programmer really thinks about these things -- annotations are (I think incontrovertibly) not how anyone already knows if/then/else to work.
I do hear you on consistency -- my personal preference would be to go in the other direction and reduce the number of other keywords defined using annotations. Again though, it's a stylistic thing, I find things generally clearer when they're not mentioned, but it's obviously easier case by case to look at something and compare.
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 personal preference would be to go in the other direction and reduce the number of other keywords defined using annotations.
I'm open to this, but we'd need to either:
- define that keywords can depend on the evaluation result of other keywords
- describe this interaction in some other way
Currently, only "presence, absence, and value" dependencies are defined (which means I need to actually add "annotation result" to that list in #1445).
If we decide to backtrack on the annotations-as-communication-mechanism thing, I'd want to be sure that all keywords can be described without them. The difficult ones are unevaluated*
.
I'm going to hold these PRs and start a discussion (linked in opening comment).
Co-authored-by: Julian Berman <[email protected]>
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 also checked if it impacted these tests, and it does not.
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'm not a fan of defining these keywords together. I think it was better defined as three separate sections with each keyword address its own concerns. I think the way then
and else
were defined were fine, but if
shouldn't mention then
and else
. if
is not dependent on then
or else
, so shouldn't mention them.
As for defining this interaction using annotations, I agree with Julian in that I don't think it's necessary and I'd rather it didn't (same for any keyword interactions that don't necessarily need be defined using annotations), but I'm reluctantly ok with defining it that way as long as we don't require implementations literally use the annotation method.
Co-authored-by: Jason Desrosiers <[email protected]>
Closing this PR as the direction has changed. These keywords may still need to be reworked, but using annotations isn't the way. |
Holding this PR based on the outcome of https://github.com/orgs/json-schema-org/discussions/491
Depends on #1445
Relates to #1444
I kinda went a bit extra on this. It seemed more appropriate (because they really work as a single unit) to specify all three together in one section rather than to keep them separate.