2020-12-patch-1-RC-0 wording issues

[gregsdennis]

additionalItems abiguity cref, L17

When we changed the specification to use annotations as the context in which some keywords behave, we included a clause that implementations which didn't use annotations may implement or optimize the processing of additionalProperties in another way which produces the same effect as the prior behaviour.

This sentence takes several reads to understand. Suggestion:

When we changed the specification to use annotations as the context in which some keywords behave, we included a clause that allowed implementations which didn't use annotations to optimize the processing of additionalProperties in another way in order to produce the same effect as the prior behaviour.

---

core, L1370

Therefore, "$id" MUST NOT contain a non-empty fragment, and SHOULD NOT contain an empty fragment.

This sounds contradictory. It may not be, but it sounds like it is. Can we add a couple examples of forbidden or unrecommended URIs?

I see that lines 1875-1877 refer to an appendix, but I don't see such invalid examples listed there.

---

validation, L444-447

Changed from

A value of 0 is allowed, but is only useful for setting a range of occurrences from 0 to the value of "maxContains". A value of 0 with no "maxContains" causes "contains" to always pass validation.

to

A value of 0 is allowed, but is only useful for setting a range of occurrences from 0 to the value of "maxContains". A value of 0 causes "minContains" to always pass validation (but validation can still fail against a "maxContains" keyword).

There is no longer any mention that minContains : 0 causes contains to pass. I think this is important as the instance may not contain an item that matches contains.

There was mention in core, L2375-2379, but maybe it bears repeating here.

[Relequestual]

Therefore, "$id" MUST NOT contain a non-empty fragment, and SHOULD NOT contain an empty fragment.

This sounds contradictory. It may not be, but it sounds like it is. Can we add a couple examples of forbidden or unrecommended URIs?

@gregsdennis This is actually just moved. This isn't a change in phrasing. Do you think we should add another appendix to give some examples? This feels a little overkill, but I'm very familiar with this. Soooo let's find out? https://twitter.com/relequestual/status/1524024474152144897

[Relequestual]

There is no longer any mention that minContains : 0 causes contains to pass. I think this is important as the instance may not contain an item that matches contains.

There [is a] mention in core, L2375-2379, but maybe it bears repeating here.

For context: This change was made in https://github.com/json-schema-org/json-schema-spec/pull/1155 by @karenetheridge, with two approvals.

I see your point, however I try to think of the specs as an implementers guide. We don't duplicate all validation concerns across keywords... that would get pretty messy, and I bet we'd miss something. I'm, against making such a change back as you're suggesting here. Open discussion.

[Relequestual]

I've made a PR which resolves your comments regarding the ADR. Otherewise, let's see where we can further discuss your comments =]

[gregsdennis]

Therefore, "$id" MUST NOT contain a non-empty fragment, and SHOULD NOT contain an empty fragment.

This sounds like

MUST NOT
https://myserver.com/foo#test
https://myserver.com/foo#/test

SHOULD NOT
https://myserver.com/foo#

implying that $id should never contain a #.

Is that right?

[gregsdennis]

We don't duplicate all validation concerns across keywords

I feel that this is an important detail regarding the interaction between contains and minContains, thus it belongs in both.

[Relequestual]

Is that right?

Yes. 100% correct.

[Relequestual]

We don't duplicate all validation concerns across keywords

I feel that this is an important detail regarding the interaction between contains and minContains, thus it belongs in both.

Not an unreasonable argument. @karenetheridge @jdesrosiers do you object to reverting this change?

As an aside, the whole "minContains changes how contains works is logical, but very frustratingly doesn't fit within the general model of how JSON Schema works. I've been trying to think how we can make it fit, but I cannot.

[gregsdennis]

doesn't fit within the general model of how JSON Schema works

Why do we adhere so strictly to the idea that keywords are completely independent? (I recognize that's why we changed the exclusive* keywords from boolean to number...)

[karenetheridge]

Not an unreasonable argument. @karenetheridge @jdesrosiers do you object to reverting this change?

What specifically do you want to revert? I scrolled through the entire diff and only found places where mention of minContains was added, but not one place where it was deleted, so it seems that the interaction between these two keywords is adequately stated. What am I missing?

Why do we adhere so strictly to the idea that keywords are completely independent?

Leaving aside that we don't, because we go against this principle multiple times at present, I think the desire to avoid keyword dependence is the complication of implementation (more positions in the schema need to be examined); also conside the complication introduced ifthe keywords are in different vocabularies (e.g. contains in applicator, and minContains in validation) -- do keywords need to be aware of what vocabulary their sister keyword is in? what if that vocabulary is not enabled by the metaschema? And is evaluation order significant (as it is for applicator and unevaluated keywords)? And now composability is affected, as minContains in a daughter schema (brought in by a sibling ref won't work the same as a sibling minContains.

[gregsdennis]

but not one place where it was deleted

Please see the last section of my opening comment.

consider the complication introduced if the keywords are in different vocabularies (e.g. contains in applicator, and minContains in validation)...

This harkens to the point I was trying to make with #1159 about reorganizing the meta-schemas.

[Relequestual]

As an aside, the whole "minContains changes how contains works is logical, but very frustratingly doesn't fit within the general model of how JSON Schema works. I've been trying to think how we can make it fit, but I cannot.

After discussion with Henry, the answer actually seems pretty obvious.

As such, what's the minimum amount of change we can do to move this over the line now, with a view to fix it in the next version properly? (There will be no functional change, but the approach will use annotations to define the interaction in a way which dosen't break our model. It's really pretty clever, but just not for this patch release.)

[jdesrosiers]

I feel that this is an important detail regarding the interaction between contains and minContains, thus it belongs in both.

I think it's fine, but it would be better if we mentioned the effect on contains as well. It's a little odd that it mentions maxContains, but not contains. I think this can be fixed with an slight update to the current text rather than reverting to the original text that doesn't mention how minContains behaves when it's value is 0. Here's my suggestion with the change highlighted.

A value of 0 is allowed, but is only useful for setting a range of occurrences from 0 to the value of "maxContains". A value of 0 causes "minContains" and "contains" to always pass validation (but validation can still fail against a "maxContains" keyword).

[gregsdennis]

I think we're missing my original point on this one. I never said anything about reverting the text. I merely pointed out

There is no longer any mention that minContains : 0 causes contains to pass.

We just need to include this aspect of the behavior.

[Relequestual]

Sorry, when I said "reverting this change", I didn't mean the WHOLE PR, just the aspect that removed what's now missing (Not even specifically reverting the text). I should have been more specific.

An update to the current text as @jdesrosiers suggests would be fine.

[gregsdennis]

That's fine. I don't think it needs reversion. I think we keep the change but add back in a mention about the interaction of the keywords.

[Relequestual]

Calling this closed as https://github.com/json-schema-org/json-schema-spec/pull/1239 was merged.

[gregsdennis]

Odd... merging the PR should have closed this automatically.