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

Split validation keywords into sections. #357

Merged
merged 1 commit into from
Aug 27, 2017

Conversation

handrews
Copy link
Contributor

@handrews handrews commented Aug 20, 2017

NOTE: This diff should be viewed with whitespace ignored

This improves the readability of the spec, particularly
new readers trying to get a feel for the scope from the
table of contents.

A previous, more complex reorganization that was attempted
before got plenty of support, and this more minimal change
avoids the issues that bogged the previous approach down.

Finally, having sections will make the section grouping
"if", "then", and "else" (#180) make more sense. Adding those
keywords without grouping them would seem odd.

This improves the readability of the spec, particularly
new readers trying to get a feel for the scope from the
table of contents.

A previous, more complex reorganization that was attempted
before got plenty of support, and this more minimal change
avoids the issues that bogged the previous approach down.

Finally, having sections will make the section grouping
"if", "then", and "else" make more sense.  Adding those
keywords without grouping the would seem odd.
@handrews
Copy link
Contributor Author

I plan to merge this as soon as I'm ready to post the if/then/else PR which depends on this one, because:

  • A far more complex change got four approvals during draft-06 work before bogging down on aspects that are not present here
  • An active community member has approved this version (making, I think, a 5th relevant approval)
  • There is no impact on JSON Schema syntax or semantics- it is purely a readability change.
  • It is easily reversed if someone comes along with an objection that would have prevented it from merging otherwise.

Generally, we have not held this sort of change to the same 2-week comment window as changes that impact implementations. This one is a bit more extensive than most such changes, but I think it's still reasonable to move forward. if/then/else will require the 2-week comment window, and it is less than 8 weeks until draft-06 expires (a soft deadline, but one I'd like to hit).

I need to update https://github.com/json-schema-org/json-schema-spec/wiki with this sort of guidance as I can't even remember exactly what we agreed on before (hence the detailed explanation in this comment).

@dlax
Copy link
Member

dlax commented Aug 23, 2017

One unrelated thing that strikes me while reading this PR is that keywords now in "Validation keywords for any instance type" subsection appear almost at the end of the "Validation" section. I tend to believe that having these before more specific ones (i.e. those applying to objects/array/etc.) would be more logical and make things easier to read. This is especially important for the ubiquitous type keyword. Thoughts?

@handrews
Copy link
Contributor Author

@dlax I almost moved it, but the difficult history of organizational PRs such as this one led me to do the absolute minimum possible. I was planning to move it later :-)

@handrews handrews merged commit 0b67e5c into json-schema-org:master Aug 27, 2017
@handrews handrews deleted the split branch August 27, 2017 23:01
dlax pushed a commit to dlax/json-schema-spec that referenced this pull request Sep 5, 2017
Sub-section "Validation keywords for any instance type" currently
appears near the end of the "Validation" section. It makes sense to have
these generic definitions before more specific ones; this is especially
true for the ubiquitous "type" keyword. So move the sub-section on top
of "Validation" section.

(See also discussions on PR json-schema-org#357.)
@gregsdennis gregsdennis added clarification Items that need to be clarified in the specification and removed Type: Maintenance labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Items that need to be clarified in the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants