Editorial: fixing a number of small issues #957
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Thank you for doing this work; concrete improvements to the spec are always welcome! As discussed at the GraphQL working group it would be helpful if you were to split this into separate pull requests each of which address a single topic. For example, a pull request that only fixed the capitalisation of the section headers would be a very easy merge, I think.
I've commented on various of the areas that I feel would require additional discussion before merging; I strongly suggest that you extract these into separate PRs so that the discussion around them can be kept on topic, and the individual changes kept minimal to reduce the effort required by reviewers having to review and re-review the changes.
|
@benjie |
You're welcome; thanks for the careful read of the spec, and your efforts to improve it 👍
There's a couple of things that I've raised alternative solutions to; see #958 and #959. Other than that, I think the rest were solid changes and you can leave them in this initial PR if you like. In future, it's a lot easier to review and merge PRs that are small and focussed, for example three PRs you could have extracted from this work would be:
This approach means that if what seemed like an easy change turns out to require discussion, it won't hold up the merging of the other changes in separate pull requests. |
Co-authored-by: Benjie Gillam <[email protected]>
Co-authored-by: Benjie Gillam <[email protected]>
|
Hi |
|
done, hopefully got it all this time |
|
Thanks for your work on this! I've approved the PR because I'm happy with all the changes; however I should note that I feel the changes in #958 should be in a separate pull request/merge (rather than incorporated here) because they touch the wording of an algoritm. Putting it in a separate PR would make it easier for people to understand the reasoning behind that specific change from the version history. |
|
done, I reverted changes for 958 |
|
shall we merge it? I have a bunch of others ready to file, with separate PRs, but would prefer start from clean 'merged' version |
|
Let me take a careful read - thanks for everyone who has do so so far. In the future, please prefer filing individual PRs for each specific change - that makes review and merge faster. My apologies if there was a miscommunication from me in my suggestion to file PRs for the changes |
|
thanks everybody for comments and feedback. |
Uses the definition syntax to set definitions for "field error" and "request error", and uses italic references in every formal normative location as well as the first reference in each prose. Include links to other sections as set up in #957 Co-authored-by: Roman Ivantsov <[email protected]>
|
Working on merging this! I'm pulling some single-thesis changes out with some description in the commit message. I'll have the last one remaining within this PR be the title casing. Reason for this is to have a clear commit per purposeful change in our commit logs. Thanks for this work and your patience! |
Uses the definition syntax to set definitions for "field error" and "request error", and uses italic references in every formal normative location as well as the first reference in each prose. This also clarifies when we previously used a plural "field errors" that we actually mean "a list of field error" Include links to other sections as set up in #957 Co-authored-by: Roman Ivantsov <[email protected]>
Uses the definition syntax to set definitions for "field error" and "request error", and uses italic references in every formal normative location as well as the first reference in each prose. This also clarifies when we previously used a plural "field errors" that we actually mean "a list of field error" Include links to other sections as set up in #957 Co-authored-by: Roman Ivantsov <[email protected]>
…raphql-main # Conflicts: # spec/Section 2 -- Language.md
Small issues in the spec:
graphql/graphql-wg#969
this PR fixes a number of issues (20), not all of them, but those that are editorial and do not require discussions