Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

gregsdennis
Copy link
Member

@gregsdennis gregsdennis commented Sep 26, 2023

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.

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
Copy link
Member

@Julian Julian Oct 2, 2023

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.)

Copy link
Member Author

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?

Copy link
Member

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).

Copy link
Member Author

@gregsdennis gregsdennis Oct 2, 2023

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.

Copy link
Member

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.

Copy link
Member Author

@gregsdennis gregsdennis Oct 2, 2023

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).

jsonschema-core.md Outdated Show resolved Hide resolved
Co-authored-by: Julian Berman <Julian@GrayVines.com>
Copy link
Member

@Relequestual Relequestual left a 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.

Copy link
Member

@jdesrosiers jdesrosiers left a 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.

jsonschema-core.md Outdated Show resolved Hide resolved
Co-authored-by: Jason Desrosiers <jdesrosi@gmail.com>
@gregsdennis
Copy link
Member Author

Closing this PR as the direction has changed. These keywords may still need to be reworked, but using annotations isn't the way.

@gregsdennis gregsdennis closed this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants