Skip to content

Reverse "contains" <-> "minContains"/"maxContains" dependency #1312

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 8, 2022
Merged

Reverse "contains" <-> "minContains"/"maxContains" dependency #1312

merged 2 commits into from
Oct 8, 2022

Conversation

handrews
Copy link
Contributor

@handrews handrews commented Sep 22, 2022 •
edited
Loading

Fixes #1311. Fixes #1161 by eliminating the confusing situation that produced the concern.

@handrews handrews added this to the draft-next milestone Sep 22, 2022
@gregsdennis
Copy link
Member

I think this wording works, but do we want to have minContains & maxContains produce an annotation that is consumed by contains? I understand the "missing keywords can't produce annotations" thing, but that's covered by the current language of assuming a min of 1. Having this annotative communication maintains consistency with how other keywords work.

@handrews
Copy link
Contributor Author

@gregsdennis this wording was intended to specify the behavior in terms of annotations, and then note an alternative (which we do for some other keywords as well). What are you looking for that isn't there? Do you want more explicit discussion of the consumption of annotations in the description of "contains"?

@gregsdennis
Copy link
Member

I was thinking that minContains and maxContains would actively produce an annotation that would be consumed by contains. This follows the model that we have established with additional* and unevaluated*.

Currently the wording you have for min/max doesn't say anything about annotations. I think this needs to be added.

Essentially, the dependency is on the annotation rather than on the other keyword directly.

@handrews
Copy link
Contributor Author

@gregsdennis

Currently the wording you have for min/max doesn't say anything about annotations. I think this needs to be added.

Lines 2530 and 2544 say:

The value of this keyword is used as its annotation result.

@gregsdennis
Copy link
Member

Okay, yeah.... I missed that.

@handrews
Copy link
Contributor Author

@gregsdennis no worries, the mentions are very brief! It actually took me a moment to find them again.

@handrews handrews mentioned this pull request Sep 25, 2022
Closed
jdesrosiers
jdesrosiers requested changes Sep 26, 2022
View reviewed changes
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.

Just one small typo, but otherwise looks good.

@handrews
"minContains"/"maxContains" to applicator vocab
ead3d31 
This moves the keywords with minamal changes to make sure the
cross-referencing (which no longer goes between two documents)
makes sense.  A subsequent commit will fix the direction
of the dependency.
@handrews
Reverse contains dependency per Karen Etheridge
5c22b30 
This fixes the problem where "minContains": 0 effectively
un-failed "contains".  The observable validation behavior
is unchanged.

Instead of "minContains" and "maxContains" reading annotations
from "contains", "contains" reads annotations from "minContains"
and "maxContains" and makes the only assertion decision.

This solution was first proposed by Karen Etheridge.
@handrews
Copy link
Contributor Author

handrews commented Oct 8, 2022

The force-push just now was to rebase and (more importantly) credit @karenetheridge for first coming up with the idea in the commit log.

@handrews handrews merged commit fb37908 into json-schema-org:main Oct 8, 2022
@handrews handrews deleted the mmcontains branch October 8, 2022 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdesrosiers jdesrosiers jdesrosiers approved these changes

@gregsdennis gregsdennis gregsdennis approved these changes

Assignees
No one assigned
Labels
Priority: High validation
Projects
None yet
Milestone
draft-next
Development

Successfully merging this pull request may close these issues.

Reverse direction of "contains" <-> "minContains" / "maxContains" dependency "Keyword is present" meaning
3 participants
@handrews @gregsdennis @jdesrosiers