some edits to the spec for the interim update release #1110
Conversation
RFC3339 refers to these as "formats". The XML xs:NCName specification refers to it as a "type".
- More cross-references added that note the dependence of these keywords on each other - Validation algorithm provided that does not rely on annotations - Confusing paragraph about logical results removed (and it is redundant with other paragraphs)
There was a problem hiding this comment.
As discussed elsewhere, let's work out a CREF for the bit about contains in the core spec, to avoid adding a formal dependency now. In addition to conceptual ickiness, IIRC cyclic references are a bit problematic when publishing spec updates. I think that's why the draft-wright-*-01 documents ref each other's -00 version instead of the -01 version.
That should probably go into its own PR as the rest here is either fine or requires minimal tweaking.
| @@ -1417,7 +1417,7 @@ | |||
| a letter ([A-Za-z]) or underscore ("_"), followed by any number of letters, | |||
| digits ([0-9]), hyphens ("-"), underscores ("_"), and periods ("."). | |||
| This matches the US-ASCII part of XML's | |||
| <xref target="xml-names">NCName production</xref>. | |||
There was a problem hiding this comment.
"production" is the correct term. Specifically, this:
NCName | ::= | Name - (Char* ':' Char*) | /* An XML Name, minus the ":" */is the NCName production in the XML Namespaces spec. In section 7, that spec says: "MUST match this specification's production for NCName.", so the terminology lines up. Type is definitely not correct, as that word has other meanings in an XML context.
So for this one we should really keep "production", although see my comments on the date-time formats for alternatives in that case.
There was a problem hiding this comment.
@karenetheridge does your thumbs up mean you agree and this should be removed from this PR?
There was a problem hiding this comment.
yes, it's an "okay, I accept this."
There was a problem hiding this comment.
OK, do you want to interactive rebase, remove the commit related to productions and force push? =]
| @@ -213,6 +213,23 @@ | |||
| <eref target="https://json-schema.org/draft/2020-12/meta/validation"/>. | |||
| </t> | |||
|
|
|||
| <section title="Keyword Independence"> | |||
| <t> | |||
| Schema keywords typically operate independently, without | |||
There was a problem hiding this comment.
I think it would be better not to copy the wording from the core spec- then it will get out of sync, and I kind of like keeping the statements of principles in the core. You can xref it, and/or just note the vocabulary-specific dependencies. But this is not a hill I intend to die on so if you feel strongly feel free to keep it.
There was a problem hiding this comment.
If the Applicator vocabulary itself has this paragraph, it's reasonable for the Validation vocabulary to have it too. How about moving it out into the general section before we start talking about individual vocabularies? Then all the exceptions don't need to be listed explicitly -- we can just give a single example, and direct the reader to pay close attention to individual keyword definitions for the exceptions.
There was a problem hiding this comment.
Well that's embarrassing. I thought it was in the general section (don't look at the change log- it's probably my fault it's not 😅 ). If you feel like factoring it out that would be great, but I do worry a bit about not highlighting the interactions sufficiently. I would be totally fine leaving this the way you have it now (with the duplicated language) and factoring it out in the next draft.
There was a problem hiding this comment.
I also didn't realize this wasn't in the general section. It definitely should be. There's no reason to duplicate documentation of this common principle. I can help with the refactor if needed.
There was a problem hiding this comment.
Agreed.
Are you happy to move this to the general section as part of this PR @karenetheridge ?
There was a problem hiding this comment.
It looks like there is a section in each applicable vocabulary (applicator, validation, unevaluated), but the content is all slightly different. Is it better to have similar-but-slightly-different wording in several places, or just one wording in one place (if this is the case, I would propose putting it in section 7.10)?
| @@ -418,6 +435,12 @@ | |||
| result is a boolean "true" and the instance array length is less than or | |||
| equal to the "maxContains" value. | |||
| </t> | |||
| <t> | |||
| If annotations are not being collected, the validation value may be determined | |||
There was a problem hiding this comment.
In terms of phrasing this should use MAY, and I feel the wording could be condensed a bit, but I do not feel strongly about that so feel free to ignore. (the condensing part- the "may" should really be a MAY).
There was a problem hiding this comment.
This paragraph is simply describing the alternative algorithm to use that doesn't involve annotations. MAY in the RFC sense isn't correct here as alternative behaviour is not permitted -- it's only the implementation can vary. How about "can"?
There was a problem hiding this comment.
I was just going by this language from the additional* applicators:
Implementations MAY choose to implement or optimize this keyword in
another way that produces the same effect, such as by directly
checking the names in "properties" and the patterns in
"patternProperties" against the instance property set.
Implementations that do not support annotation collection MUST do so.
As long as they end up consistent I'm probably fine with whatever. If making it consistent gets bogged down, I'll live with a little inconsistency. It is probably better to use "can" than lower-case "may", although that is definitely not an absolute rule.
There was a problem hiding this comment.
Sounds like a reaonsable change. Let's go with "can".
There was a problem hiding this comment.
I'm cutting out this entire part of the revision, since (as per other PRs recently submitted) trying to define a parallel implementation using annotations is not going to be identicaly, so we shouldn't suggest it at all.
| @@ -701,30 +731,34 @@ | |||
| <list style="hanging"> | |||
| <t hangText="date-time:"> | |||
| A string instance is valid against this attribute if it is | |||
| a valid representation according to the "date-time" production. | |||
There was a problem hiding this comment.
As before, production is correct, although I think "rule" is also correct as it's what is used in the RFC for ABNF, so perhaps that would be more agreeable?
Specifically, date and time are from these ABNF rules in section 5.6 of RFC 3339:
date-fullyear = 4DIGIT
date-month = 2DIGIT ; 01-12
date-mday = 2DIGIT ; 01-28, 01-29, 01-30, 01-31 based on
; month/year
time-hour = 2DIGIT ; 00-23
time-minute = 2DIGIT ; 00-59
time-second = 2DIGIT ; 00-58, 00-59, 00-60 based on leap second
; rules
time-secfrac = "." 1*DIGIT
time-numoffset = ("+" / "-") time-hour ":" time-minute
time-offset = "Z" / time-numoffset
partial-time = time-hour ":" time-minute ":" time-second
[time-secfrac]
full-date = date-fullyear "-" date-month "-" date-mday
full-time = partial-time time-offset
date-time = full-date "T" full-timeand duration comes from RFC 3339 Appendix A, ported from the ISO spec:
dur-second = 1*DIGIT "S"
dur-minute = 1*DIGIT "M" [dur-second]
dur-hour = 1*DIGIT "H" [dur-minute]
dur-time = "T" (dur-hour / dur-minute / dur-second)
dur-day = 1*DIGIT "D"
dur-week = 1*DIGIT "W"
dur-month = 1*DIGIT "M" [dur-day]
dur-year = 1*DIGIT "Y" [dur-month]
dur-date = (dur-day / dur-month / dur-year) [dur-time]
duration = "P" (dur-date / dur-time / dur-week)There was a problem hiding this comment.
How about "date-time" ABNF rule?
FWIW that would also resolve the previous confusion I'd raised about duration, where the ABNF rule in RFC3339 is very much incomplete compared to the actual ISO 8601 definition.. so we can be specific that we're only targeting what's in the RFC. (We can also optionally validate the remainder of the ISO 8601 definition, as those cases are explicitly omitted from the test suite.)
There was a problem hiding this comment.
@karenetheridge I'm confused, duration already cites the ISO rule as quoted in the RFC. If it's not in the RFC, we are not referencing it. All we are doing is referencing that production. That's why the word "production" was used.
I think "rule" (with the RFC section number, which is already there) is sufficient, although I would not object to ABNF so that's fine.
There was a problem hiding this comment.
I find the use of the word "production" to be very confusing and would prefer we avoid using it unless it's referencing something very specific, in which case we should link to its definition somewhere in the preamble like we do for MUST/MAY/RECOMMENDS etc.
There was a problem hiding this comment.
The tone of the comment I made here was too condescending. I am leaving this up for transparency so anyone who wants to can look at the edit history. I am otherwise withdrawing the comment.
| @@ -213,6 +213,23 @@ | |||
| <eref target="https://json-schema.org/draft/2020-12/meta/validation"/>. | |||
| </t> | |||
|
|
|||
| <section title="Keyword Independence"> | |||
| <t> | |||
| Schema keywords typically operate independently, without | |||
There was a problem hiding this comment.
Agreed.
Are you happy to move this to the general section as part of this PR @karenetheridge ?
| @@ -418,6 +435,12 @@ | |||
| result is a boolean "true" and the instance array length is less than or | |||
| equal to the "maxContains" value. | |||
| </t> | |||
| <t> | |||
| If annotations are not being collected, the validation value may be determined | |||
There was a problem hiding this comment.
Sounds like a reaonsable change. Let's go with "can".
| @@ -437,10 +460,17 @@ | |||
| annotation result is a boolean "true" and the instance array length is | |||
| greater than or equal to the "minContains" value. | |||
| </t> | |||
| <t> | |||
| If annotations are not being collected, the validation value may be determined | |||
|
closed in favour of #1155. |
I had some notes on things that could be touched up in the current spec.
I'm happy to split some of these off in a separate PR if they need more discussion (or someone with push rights to the repo can just cherry-pick the others).