Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Another attempt at clarifying objects/arrays. #234

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

Closed
handrews wants to merge 6 commits into json-schema-org:master from handrews:sections

Conversation

Copy link
Contributor

@handrews handrews commented Jan 25, 2017

NOTE: I'm putting this up because @awwright and I both thought that my last attempt to clarify this still wasn't quite getting it. Since this moves a lot of things around the diff looks much worse than the changes actually are. It's better to grab this XML file and build it locally to see the changes.

While I like this approach better, I do not feel strongly enough about it to insist on it (or anything like it) going in for Draft 06. I'm just posting it as an idea, and if it doesn't get support quickly I'll be happy to drop it.


This goes back to Draft 04's approach of grouping keywords,
but mostly retaining Draft 05's more concise wording and
per-keyword organization.

The general considerations section on validating primitive types
and child values has been simplified, and the details have been
moved down to each grouping of array or object keywords. This
puts the information near where it is most relevant, which
allows removing some of the more confusing working in the
keywords that apply to child values.

Copy link
Contributor Author

To make it easier to see the restructuring without having to build locally, here is what the table of contents looks like with this change:

Table of Contents
1. Introduction
2. Conventions and Terminology
3. Interoperability considerations
 3.1. Validation of string instances
 3.2. Validation of numeric instances
 3.3. Regular expressions
4. General validation considerations
 4.1. Keywords and instance primitive types
 4.2. Validation of primitive types and child values
 4.3. Constraints and missing keywords
 4.4. Keyword independence
5. Meta-schema
6. Validation keywords
 6.1. Validation keywords for numeric instances (number and integer)
 6.1.1. multipleOf
 6.1.2. maximum
 6.1.3. exclusiveMaximum
 6.1.4. minimum
 6.1.5. exclusiveMinimum
 6.2. Validation keywords for strings
 6.2.1. maxLength
 6.2.2. minLength
 6.2.3. pattern
 6.3. Validation keywords for arrays as primitive types
 6.3.1. maxItems
 6.3.2. minItems
 6.4. Validation keywords for array contents
 6.4.1. contains
 6.4.2. uniqueItems
 6.5. Validating array elements with subschemas
 6.5.1. items
 6.5.2. additionalItems
 6.6. Validation keywords for objects as primitive types
 6.6.1. maxProperties
 6.6.2. minProperties
 6.6.3. propertyNames
 6.6.4. required
 6.7. Validating object properties with subschemas
 6.7.1. properties
 6.7.2. patternProperties
 6.7.3. additionalProperties
 6.8. Conditional validation based on object properties
 6.8.1. dependencies
 6.9. Validation keywords for any instance type
 6.9.1. enum
 6.9.2. const
 6.9.3. type
 6.9.4. allOf
 6.9.5. anyOf
 6.9.6. oneOf
 6.9.7. not
 6.10. definitions
7. Metadata keywords
 7.1. "title" and "description"
 7.2. "default"
 7.3. "examples"
8. Semantic validation with "format"
 8.1. Foreword
 8.2. Implementation requirements
 8.3. Defined formats
 8.3.1. date-time
 8.3.2. email
 8.3.3. hostname
 8.3.4. ipv4
 8.3.5. ipv6
 8.3.6. uri
 8.3.7. uri-reference
 8.3.8. uri-template
 8.3.9. json-pointer
9. Security considerations
10. References
 10.1. Normative References
 10.2. Informative References
Appendix A. Acknowledgments
Appendix B. ChangeLog
Authors' Addresses

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 think this would make learning JSON Schema a bit easier. So I'm for it.

Copy link
Contributor Author

@epoberezkin what's your thought on this? You'll want to use this link for the diff, the regular diff is hard to read: https://github.com/json-schema-org/json-schema-spec/pull/234/files?w=1

Copy link
Member

@handrews seems ok. It seems consistent with draft-4 as schema can be boolean, am I right? I only have two notes:

  • I'd rather we use "true" schema (or schema equal to "true" etc.) instead of an empty schema - the reason to introduce boolean schemas was exactly to allow that in schemas, so it makes sense to use it in I-D as well (however we refer to it).
  • The separation of TOC for arrays could be made consistent with that for objects, i.e either 6.3. and 6.4. are merged or 6.6 is split (in which case maxProperties and minProperties belong to "primitive" group and propertyNames and required belong to "contents" group). I think merging 6.3 and 6.4 is somewhat better.

Copy link
Contributor Author

The separation of TOC for arrays could be made consistent with that for objects, i.e either 6.3. and 6.4. are merged or 6.6 is split (in which case maxProperties and minProperties belong to "primitive" group and propertyNames and required belong to "contents" group). I think merging 6.3 and 6.4 is somewhat better.

Hmm.. interesting. I split both arrays and objects up as follows:

  1. Things that validate the structure of the array or object (array positions and property names and counts) that do not involve the values within the container at all.

  2. Things that required examining all values within the container and making a decision based on that examination. Only arrays have keywords of this nature (technically for "contains" you can stop examining values as soon as you find it, but you potentially have to examine all).

  3. Things that are only about applying subschemas to values.

The reason for this split is the difference in implementing each type. You can validated "maxProperties", "minProperties", "required", and "propertyNames" all without looking at a single value in the object. That is not true of "contains" and "uniqueItems". Those two array keywords have some unique implementation challenges as a result. So the division is doing exactly what I intended it to do.

Relequestual reacted with thumbs up emoji

Copy link
Member

I'm not sure I see the point of splitting up keywords into "Validating object properties with subschemas" versus "Validation keywords for objects as primitive types", that doesn't help me navigate the TOC at all and it doesn't help clarify the behavior at all.

I don't even really understand why "propertyNames" is on one list, but "additionalProperties" is on a different one. They're both describing different slots in a 2-tuple.

Copy link
Contributor Author

I don't even really understand why "propertyNames" is on one list, but "additionalProperties" is on a different one. They're both describing different slots in a 2-tuple.

I have no idea what you even mean by this. Property name constrains the set of legal names. additionalProperties is unrelated- it tells you what subschema to apply to child values of property names that are neither found in "properties" nor match any pattern in "patternProperties".

Applying subschemas to children is a fundamentally different operation than everything else. The implementation is very different. And it is the piece of validation that is needed to do anything else- JSON Schema as a media type is useless without it or a functional equivalent.

Copy link
Contributor Author

In all seriousness, the conflation of the child schema rules with other sorts of validation was one of the biggest sources of confusion on past projects. Clarifying the different types of behavior is immensely helpful in my experience. @Relequestual and @epoberezkin seem to agree (aside from a much less significant question over how to handle "uniqueItems" and "contains").

Can you explain why you think it is helpful to munge them all together? I know that I have very much disliked dealing with Draft 05's flat namespace. I find that draft much harder to read and reason about from a high level because there is no way to apply concepts to groups of similar keywords

Copy link
Member

@handrews

Applying subschemas to children is a fundamentally different operation than everything else.

From this point uniqueItems that doesn't involve validation at all should be in 6.3 and contains in 6.5. contains is not different from items with a single schema (they are the related as "or" and "and").

Also, headings for the sections can be confusing ("primitive" vs "contents") - the criteria I quoted above could have been used ("applying schemas to children"). RFC7159 doesn't even refer to arrays and objects as "primitive types", they are called "structured types": https://tools.ietf.org/html/rfc7159#section-1

Copy link
Contributor Author

@epoberezkin

  • uniqueItems does involve validation- it returns a validation true or false . It validates a condition across the entire array contents which is unusual. It (and contains) are the only ones where you have to look at the entire array contents as a whole.

  • "contains" does not assign a child schema to a specific element. It just requires that some element across the array (again examining the array as a whole) matches.

I could see grouping "contains" with items based on your OR observation, and (although I really don't like it because the implementation mechanism is completely different) grouping "uniqueItems" with min/maxItems. If it will get you to approve the change overall I could do that- would that be sufficient or do you have other blocking objections? I'm not going to rewrite the whole thing without some assurances.

  • the "primitive types" language has been here for a long time. The other way I've seen it phrased is "container types". I can't fix everything at the same time, though, so can we focus on the things that are really changed.

Copy link
Member

Regarding @epoberezkin's objections, I can see your argument for splitting up 6.4. To me I could see 6.3 actually having two subsections, which would contain the contents of 6.3 and 6.4. BUT I don't think it's needed, and I agree with @handrews's assessment of grouping based on probably implementation methods.

@awwright I don't see any specific objections, just queries in your comments, so I would see your comments as a +/- 0. I consider my PR approval to be a +1 on this, and I can see a clear benefit to my ability to understand the specification.

@epoberezkin could we agree that this PR is a step in the right direction, and then file a further issue to seek to further address grouping, where we can agree on how grouping should be systematically determined?

handrews reacted with heart emoji

Copy link
Contributor Author

handrews commented Feb 5, 2017

Updated to remove the phrase "child validation"- as @awwright observed in PR #247 there is no such distinct thing.

Copy link
Member

awwright commented Feb 6, 2017

I'm still really confused on the distinction this is making.

The only thing that validation keywords do are return "valid"/"invalid", and they all operate the same way like this. Many of the keywords additionally take arguments, and some of them take schemas as arguments, but this doesn't warrant dividing them up into sections based on this property.

Copy link
Contributor Author

handrews commented Feb 6, 2017

but this doesn't warrant dividing them up into sections based on this property.

That's your opinion. The thread so far shows that I am not the only one who thinks otherwise. @Relequestual has indicated agreement, and while @epoberezkin objects to splitting "contains" and "uniqueItems" from "maxItems" and "minItems" (I'm OK with putting them in the same section if that moves this forward) and doesn't like the term "primitive types" (I'm OK with using another term), he has not objected to the fundamental point of discussion keywords that apply subschemas to child instances separately (in fact, he said it "seems OK").

So while this is not a vote, you can't just blanket assert that all of our opinions are irrelevant.

This is a spec that we are writing, not a description of absolute truth. Considering how subschemas and child instances are matched as its own process is a more useful mental model for most of us than the way you talk about this.

Copy link
Member

awwright commented Feb 7, 2017
edited
Loading

@handrews Not to discount people's thoughts in the least, but the kinds of thoughts I need are reasons and rationale.

What benefit are we getting from this? Why would I want to refer someone to any of the sections in question?

Also, this introduces language like

Validation of the complete instance succeeds if validation of the primitive type and validation of each child instance succeeds.

Yet this isn't how validation works: Validation succeeds if every keyword succeeds; nothing more, nothing less.

Copy link
Contributor Author

handrews commented Feb 7, 2017

Yet this isn't how validation works: Validation succeeds if every keyword succeeds; nothing more, nothing less.

  1. That language is easily fixed if needed.
  2. We're writing the spec. We can define how validation work in whatever way makes the most sense for implementors and schema authors.

What benefit are we getting from this? Why would I want to refer someone to any of the sections in question?

  1. Organization. Giant flat lists of sections make it hard to visually grasp what's there, what's related and what's not.
  2. Clarity. The idea that many validation keywords only apply to certain types, meaning that it is only possible for them to fail with instances of that type, has been a source of confusion. Grouping keywords by type emphasizes this.
  3. Common behavior. Having sections allows describing shared behavior near where it is relevant, instead of all the way at the top of the spec. We could really drop 4.1 and 4.2 entirely and instead describe the relevant principles together with the keywords that demonstrate them. This way, when someone looks up the keywords, they are much more likely to see the related principles.
  4. Matching subschemas to child instances is inherently different both conceptually and in implementation, and should be clearly called out as such. It is a key mechanism on which all other possible vocabularies depend. In order to correctly support additional vocabularies such as hyper-schema, you need to implement those groups of keywords even if you are not supporting validation. It produces a fundamentally different code organization.

@Relequestual since you indicated that you found this helpful could you please add your thoughts?

Copy link
Member

Grouping the validation keywords into groups based on what things they validate against is really helpful if you're new to the spec and can't spend a few days reading, or if you've used it previously, and need to use the spec as reference but you've forgotten how things work.

Consider, I'm new to the spec and I wan't to know what validation I can apply to a string. Previously I would have to read each validation key word to work out if it can be applied to a string or not. Now I can clearly see that only a few words can apply validation constraints to strings.

Consider, I have an existing schema that was made by someone else, and it uses pattern, but I'm not familiar with what types of things it applies to. Because it's clearly sectioned in the "keywords which apply to strings" section, I immediatly know that pattern applies to strings

In both these scenarios, you've just made using JSON Schema a lot more accessible, lowering the barrier to entry by making it easier to understand some things at a glance without the need to read all the validation keywords.

handrews reacted with heart emoji

handrews and others added 2 commits February 9, 2017 13:43
This goes back to Draft 04's approach of grouping keywords,
but mostly retaining Draft 05's more concise wording and
per-keyword organization.
The general considerations section on validating primitive types
and child values has been simplified, and the details have been
moved down to each grouping of array or object keywords. This
puts the information near where it is most relevant, which
allows removing some of the more confusing working in the
keywords that apply to child values.
The "child validation" phrasing could be interpreted as indicating
a separate validation process, which is incorrect. Removed all
remaining references to "child validation".
Removed superfluous paragraphs about primitive value validation
always succeeding (left over from the approach taken in Draft 04).
Removed the general considerations section about primitive vs
child validation, as it is made clear by grouping keywords into
sections and describing the behavior of each section.
Moved description of type-specific validation behavior to each
type-specific section, and removed the general considerations section
about that behavior.
Copy link
Contributor Author

handrews commented Feb 9, 2017

OK, in addition to rebasing this and incorporating other wording changes that were committed in the meantime, I have:

  • Removed all references to "child validation"
  • Removed paragraphs that talk about "validation of the primitive instance always succeeds" (that was a leftover Draft 04 concept, actually)
  • Noted the applicability of keywords to type in each of the per-type keyword sections, and removed the general considerations section on that topic
  • Removed the general considerations section about primitive vs child instance validation, as that is now all covered more clearly in the keyword sections for applying subschemas to array elements or object property values

I think this preserves my intent for organizational improvements and clarity while avoiding the specific phrasings that @awwright found objectionable.

Copy link
Contributor Author

@epoberezkin does the current update work for you? I got rid of the "primitive type" language that you didn't like, and followed your suggestion to merge 6.3 and 6.4.

We can replace empty arrays with true in a separate PR, that covers a bit more ground than this change anyway. Are you willing to approve this as-is?

Focus on subschema vs not subschema, no matter how the
subschema is used.
Copy link
Contributor Author

handrews commented Feb 15, 2017
edited
Loading

After discussing this further with @awwright on IRC, while neither of us have entirely convinced the other, I agreed that "propertyNames" and "dependencies", as keywords with subschemas, should be grouped with the other object-related subschema keywords. I've tried out a few different options. Here is one keeping the rough order, but simplifying the section titles describing the subschema keywords, and also calling out the logical operator keywords as they are the last group that applies subschemas:

This is the Single-level organized by type and subschema use option.

Table of Contents
1. Introduction
2. Conventions and Terminology
3. Interoperability considerations
 3.1. Validation of string instances
 3.2. Validation of numeric instances
 3.3. Regular expressions
4. General validation considerations
 4.1. Constraints and missing keywords
 4.2. Keyword independence
5. Meta-schema
6. Validation keywords
 6.1. Validation keywords for numeric instances (number and integer)
 6.1.1. multipleOf
 6.1.2. maximum
 6.1.3. exclusiveMaximum
 6.1.4. minimum
 6.1.5. exclusiveMinimum
 6.2. Validation keywords for strings
 6.2.1. maxLength
 6.2.2. minLength
 6.2.3. pattern
 6.3. Validation keywords for arrays
 6.3.1. maxItems
 6.3.2. minItems
 6.3.3. uniqueItems
 6.4. Validating array elements with subschemas
 6.4.1. items
 6.4.2. additionalItems
 6.4.3. contains
 6.5. Validation keywords for objects
 6.5.1. maxProperties
 6.5.2. minProperties
 6.5.3. required
 6.6. Validating objects with subschemas
 6.6.1. properties
 6.6.2. patternProperties
 6.6.3. additionalProperties
 6.6.4. propertyNames
 6.6.5. dependencies
 6.7. Validation keywords for any instance type
 6.7.1. enum
 6.7.2. const
 6.7.3. type
 6.8. Applying subschemas with boolean logic
 6.8.1. allOf
 6.8.2. anyOf
 6.8.3. oneOf
 6.8.4. not
 6.9. definitions
7. Metadata keywords
 7.1. "title" and "description"
 7.2. "default"
 7.3. "examples"
8. Semantic validation with "format"
 8.1. Foreword
 8.2. Implementation requirements
 8.3. Defined formats
 8.3.1. date-time
 8.3.2. email
 8.3.3. hostname
 8.3.4. ipv4
 8.3.5. ipv6
 8.3.6. uri
 8.3.7. uri-reference
 8.3.8. uri-template
 8.3.9. json-pointer
9. Security considerations
10. References
 10.1. Normative References
 10.2. Informative References
Appendix A. Acknowledgments
Appendix B. ChangeLog
Authors' Addresses
epoberezkin reacted with thumbs up emoji

Copy link
Contributor Author

@awwright if having two side-by-side sections each for "objects" and "arrays" is the main sticking point, I'd be happy to have one section for each with two subsections. Perhaps that would make it less confusing than having them be two peer sections at the same level?

Copy link
Member

epoberezkin commented Feb 15, 2017
edited
Loading

does the current update work for you?

@handrews yes, looks good now and I agree with the proposed structures - everything having subschemas applied to elements grouped together... I wrote above as well that difference between "items" and "contains" is no greater than between "allOf" and "anyOf", so it is only logical that they are in the same group.

Copy link
Member

I'm still happy with this PR after the above changes. To me the benefits are clear. Unless there are any clear negatives, I feel we should merge.

Copy link
Contributor Author

OK it's a little unclear to me what exactly @epoberezkin is endorsing (I think not the way I have it in the PR right now but instead one of the other proposals). I'm going to name the proposal as the PR currently stands, which is show as a table of contents 4 comments above this, the "Single-level organization by type and subschema use", and paste two alternative table of contents in further comments (although I will NOT update the PR diff again until we've chosen one of the three.

Copy link
Contributor Author

handrews commented Feb 15, 2017
edited
Loading

This is the Two-level organized first by type, then by subschema use option, which I'm showing to address @awwright's concern that having two object and two array sections side-by-side is puzzling (I also grouped the two universal keyword sections):

Table of Contents
1. Introduction
2. Conventions and Terminology
3. Interoperability considerations
 3.1. Validation of string instances
 3.2. Validation of numeric instances
 3.3. Regular expressions
4. General validation considerations
 4.1. Constraints and missing keywords
 4.2. Keyword independence
5. Meta-schema
6. Validation keywords
 6.1. Validation keywords for numeric instances (number and integer)
 6.1.1. multipleOf
 6.1.2. maximum
 6.1.3. exclusiveMaximum
 6.1.4. minimum
 6.1.5. exclusiveMinimum
 6.2. Validation keywords for strings
 6.2.1. maxLength
 6.2.2. minLength
 6.2.3. pattern
 6.3. Validation keywords for arrays
 6.3.1. Basic array validation keywords
 6.3.1.1. maxItems
 6.3.1.2. minItems
 6.3.1.3. uniqueItems
 6.3.2. Validating array elements with subschemas
 6.3.2.1. items
 6.3.2.2. additionalItems
 6.3.2.3. contains
 6.4. Validation keywords for objects
 6.4.1. Basic object validation keywords
 6.4.1.1. maxProperties
 6.4.1.2. minProperties
 6.4.1.3. required
 6.4.2. Validating objects with subschemas
 6.4.2.1. properties
 6.4.2.2. patternProperties
 6.4.2.3. additionalProperties
 6.4.2.4. propertyNames
 6.4.2.5. dependencies
 6.5. Validation keywords for any instance type
 6.5.1. Basic universal validation keywords
 6.5.1.1. enum
 6.5.1.2. const
 6.5.1.3. type
 6.5.2. Applying subschemas with boolean logic
 6.5.2.1. allOf
 6.5.2.2. anyOf
 6.5.2.3. oneOf
 6.5.2.4. not
 6.6. definitions
7. Metadata keywords
 7.1. "title" and "description"
 7.2. "default"
 7.3. "examples"
8. Semantic validation with "format"
 8.1. Foreword
 8.2. Implementation requirements
 8.3. Defined formats
 8.3.1. date-time
 8.3.2. email
 8.3.3. hostname
 8.3.4. ipv4
 8.3.5. ipv6
 8.3.6. uri
 8.3.7. uri-reference
 8.3.8. uri-template
 8.3.9. json-pointer
9. Security considerations
10. References
 10.1. Normative References
 10.2. Informative References
Appendix A. Acknowledgments
Appendix B. ChangeLog
Authors' Addresses

Copy link
Contributor Author

@awwright @epoberezkin @Relequestual could you each please indicate which of the three options present, if any, are preferred or at least acceptable to you?

  • one-level, organized by both type and subschema usage
  • two-level, organized first by type, then by subschema usage
  • two-level, organized first by subschema usage, then by type

Copy link
Member

epoberezkin commented Feb 15, 2017
edited
Loading

  1. two-level, organized first by subschema usage, then by type

@handrews this makes the least sense to me, between the first two it doesn't matter to me (as long as the first option means that it is ordered by type first)

Copy link
Contributor Author

@epoberezkin thanks, I had misread your earlier comment to mean that you supported option 3 so I am glad that this clears that up. Unless someone makes a convincing argument for that option I am not attached to it, I put it up here just because I thought that's what you were proposing.

I am also happy with options 1 or 2, with a very slight preference for option 2. But only very slight.

Copy link
Member

I would also vote for option 2, but I also feel 3 is acceptable.

Copy link
Contributor Author

OK, since option 2 is the consensus among at least three of us, I will update the PR with that option. @awwright please comment on whether you'd find option 2 acceptable (we can of course continue to tweak the wording along the lines of #247 after this goes in, but it will be easier to manage diffs after this organizational change)

For object, array, and universal validation keywords, group first by
type applicability, then by whether the keywords take a subschema
or not.
Note the use of object and array subschema keywords to apply other
vocabularies.
Considered to still be useful as an introduction to the design
principle involved.
Seems to have been changed in a rebase error.
Copy link
Contributor Author

Conversations with @awwright on IRC concluded with unresolvable differences.

Copy link
Member

Nothing unresolvable; I'm gonna break up some of the changes proposed here for separate discussion.

Copy link
Contributor Author

@awwright requiring that I take two trivial changes that we both agree on anyway out into separate PRs and then rebase a complex reformatting of the file onto that (rebasing file reorganizations pretty much just makes for merge conflicts all over) is an unresolvable problem for me. I am just not going to do that much work to get to the same end result after three people here and an additional person on IM have had no trouble with understanding, reviewing, and approving the change.

Your combination of an inflexibly dogmatic approach to contributions while also refusing to document your unbendable requirements in a contributors' guide shows that you do not respect the efforts of collaborators and have no interest in working out any sort of compromise on work already done.

If you continue on this path I will not be the last potential contributor that you alienate. That does not bode well for JSON Schema as a viable project.

Copy link
Member

The guideline, as provided on the Wiki, is that PRs should solve a particular issue, so I think it's fair to ask for things to be broken apart in the face of scope creep.

In the course of this, I would like to ask that we put first things first. Maybe in some cases the end result is the same, but often it's not. If the motivation for the change is that an earlier draft is confusing, I have to take a close look and make sure that confusion is present in the current work, instead of fixing a misconception about an old draft.

In all cases, it makes it makes changes easier to reason about, it helps identify areas of agreement, it ensures that all of the changes are reviewed with the detail they deserve (a lot of things slipped through until today!), and it helps forward compatibility (especially for software).

I can perform these edits myself, of course, especially for cases where it might be asking a lot.

The standards process is designed to be comprehensive, even to the detriment of other things, so this isn't always going to get to go the way everyone wants, or even the way a majority of people want. We've also adopted a process that is a lot more invested than how most WGs operate (where the editor just writes everything). Please keep in mind the effects of that.

Copy link
Contributor Author

If you had been able to identify a single thing that produced a different end result or made it difficult to reason about, then I would have some sympathy. In fact I offered to rework things if you found such a problem.

But the things you are insisting made it impossible to go forward are:

  • Fixing a JSON syntax error by adding a single comma
  • Replacing "child validation" with "validation", which was something you wanted to do anyway

Everything else you pointed out I changed as you asked, immediately.

You don't actually talk about problems. If you did, we could resolve them. The above two points were not problems that needed resolving, and you did not even attempt to explain what problems they presented other than "first things first". It's not even clear why those have to be first. You keep going on about ways that things could be hard to reason about, but you never actually show anything that is hard to reason about. If you had, I would have addressed it.

You originally wouldn't even talk about the PR but demanded "rationales". When @Relequestual and I provide them you wouldn't reply. You just demanded rationales again. I repeatedly asked you to reply today on IRC and you ignored me every single time.

You have a dogmatic idea of what a standards process is like, yet I've been unable to find any evidence of you shepherding a standard to publication in the past. You are absolutely certain that you are in the right in every way and unwilling to discuss options. If you want us to just do what you say, that has to be earned.

I added things to the PR because they were things you objected to, or were trivial fixes (a single comma!!!). But now you object to them being there. I've tried doing what you said, and the result was that you just changed what you said to something else. My unwillingness to jump through your hoops is the direct result of your past behavior.

If you want a different outcome, try compromise and actually discussing real things instead of possibilities. Try working with what is here instead of spreading FUD about what might happen if your approach isn't followed exactly.

Copy link
Member

Everything else you pointed out I changed as you asked, immediately.

I believe this is a misunderstanding. The only thing I asked for was a rationale for several things, or otherwise I pointed out some things that don't make sense to me; these are attempts to understand where you're coming from, not requests for changes to the PR.

Now, if you think I've identified something that you believe needs fixing, of course you can make a fix. However, if this problem is something that already existed and isn't directly related to the PR (i.e. isn't introduced with the patch), it would be best to create a new PR in the interest if keeping things topical.

Apologies for not identifying these points sooner, and not being as responsive as I could have been over the course of this PR.

Try working with what is here instead of spreading FUD about what might happen if your approach isn't followed exactly.

Please try to keep comments pertinent to the issue. I'm trying to constructively identify why it's a good idea to deal with underlying issues first, before factoring/refactoring, even if it doesn't otherwise seem necessary.

Note, through the course of our discussion I identified several things (especially on IRC today) that apparently made it in that were never intended to be changed at all. Can you see how, as I'm reading though the patch, it might be somewhat confusing trying to figure out how these changes are relevant?

I'll keep pushing some more PRs to deal with the issue being raised; stating I don't understand some things about this, and splitting up the PR, isn't by any stretch trying to kill it off.

Copy link
Contributor Author

You're just repeating the same things now. They are no more convincing this time around.

The point of review is to identify things such as bits that got in due to a rebase error (the "enum") due to how long this PR was left unattended. We all make errors, that's why we review. Then we fix the errors and move forwards. It's not a reason to declare everything invalid and impossible to think about.

As for rationales, you keep ignoring the rationales @Relequestual and I already posted, and ignored every time here or on IRC that I have pointed out that we already provide rationales, so I don't see any point to providing more.

There's no further productive discussion to be had here.

Copy link
Member

@awwright for my benefit, as I am a bit lost here, could you please tell me what changes, exactly, you want to be moved into a separate PR?

I would really appreciate if you could put the links to the lines in this PR and identify the changes that should be separated.

Copy link
Member

awwright commented Feb 18, 2017
edited
Loading

@epoberezkin I've created two PRs #255 and #256 to deal with some issues that are really separate from the purpose of the PR.

This PR originally proposed other changes, like a change to the abstract, changes of paragraphs, and a keyword definition, that I was going to split off as well; these seem to have been simple mistakes.

Generally, separate conserns should go in separate PRs, so I'm still trying to figure out which ones are related.

Copy link
Contributor Author

And apparently after a full 24 hours of "trying to figure out which are related" you can't even come up with any questions to ask, much less problems to identify.

To clarify:

  • The change to the abstract was one I had put in on purpose, but didn't care much about one way or the other. You didn't want it there, I took it out, we moved on.

  • The "enum" change was a rebase error because this PR languished so long that I had to rebase it on top of a bunch of other changes which is a giant pain in the ass. This is a very common type of error and one of the reasons we do reviews, so acting like it somehow makes the whole change a problem is baffling. You caught it, I fixed it, we moved on .

  • The section from the "General Considerations" that I removed was definitely an intentional part of the change. But you indicated that you thought it was still useful- fine with me, I put it back in and we moved on.

But I drew the line when you declared that fixing a single comma and changing "child validation" to "validation" made the PR unmanageable. You have still never made the case for that. Yes, you can say that the comma has nothing to do with the main point, but it's a comma. It does not confuse anything. The word change was agreed upon earlier, and does not interfere with anything. There is no need to do extra work for this unless you found some other change that actually is tangled up with the others.

Which you have not. You just blather on about how everything has to be perfectly separated, even though you cannot show a single example of a problem with this PR.

Copy link
Member

awwright commented Feb 18, 2017
edited
Loading

@handrews I realize it's a lot of work sometimes, but please be mindful of the tone. This is only a PR.

I've asked many questions, but the only request I've made is that a PR be single issue. Unfortunately, this wasn't something that was immediately clear to me. I apologize for the confusion.

The three examples you provided are times the PR changed because I asked a question (or remarked I didn't understand). When the PR changes every time I ask a question, it becomes hard to understand what the intent is.

Here's some other lines that seem to make language changes:

https://github.com/json-schema-org/json-schema-spec/pull/234/files?w=1#diff-2391a2ef934e9aed11cbf87a65eb61c9L175
https://github.com/json-schema-org/json-schema-spec/pull/234/files?w=1#diff-2391a2ef934e9aed11cbf87a65eb61c9L455
https://github.com/json-schema-org/json-schema-spec/pull/234/files?w=1#diff-2391a2ef934e9aed11cbf87a65eb61c9L469
https://github.com/json-schema-org/json-schema-spec/pull/234/files?w=1#diff-2391a2ef934e9aed11cbf87a65eb61c9L573
https://github.com/json-schema-org/json-schema-spec/pull/234/files?w=1#diff-2391a2ef934e9aed11cbf87a65eb61c9L650

If any of these make sense to include in a separate PR, then please do so.

I'm happy to write organizational changes after we identify specific language/paragraph changes.

Copy link
Contributor Author

I'm going back to "There's no further productive discussion to be had here," which is where I should have stopped. I only spoke up again because of @epoberezkin.

@awwright @epoberezkin @Relequestual if any of you want to pick up these changes in any form, please be my guest. Otherwise, I closed the PR. I'm leaving it closed. I don't expect any further action, and will not be following up.

@handrews handrews deleted the sections branch August 27, 2017 23:03
@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
Reviewers

@Relequestual Relequestual Relequestual approved these changes

Assignees
No one assigned
Labels
clarification Items that need to be clarified in the specification Priority: Medium
Projects
None yet
Milestone
draft-6
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /