-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Draft] Define stages for changes to GraphQL specification #342
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting the ball rolling on this one @IvanGoncharov. I left a few small comments inline in the way of initial feedback. I think the one thing that is missing here is a framing statement at the top to provide a little more context: in particular I'd like to see it called out that this is intended to:
- Provide a clear process for getting medium-to-large changes into the spec.
- Provide transparency for participants in the GraphQL ecosystem into the status/progress of in-flight proposals.
And that it's explicitly not intended to slow down the application of small fixes (eg. grammar or spelling typos, minor "bug fixes" etc).
(Or something like that. Either words like these or something similar to provide context.)
stages.md
Outdated
## Stage 2: Candidate | ||
**Prerequisite**: | ||
+ Prepared PR for graphql-js | ||
+ Notify all members of GraphQL WG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably link to the WG repo here.
stages.md
Outdated
## Stage -2: proposed change(optional) | ||
**Prerequisite**: Described problem/change should be specific to the content of GraphQL Specification and not be an implementation detail. | ||
|
||
**Purpose**: Filter out question, issues for other repos and engage community discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question → questions
stages.md
Outdated
## Stage 0: Proposal | ||
**Prerequisite**: | ||
+ Initial version of spec changes | ||
+ Filled checklist in PR description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you have in mind for the checklist?
stages.md
Outdated
## Stage 1: Draft | ||
**Prerequisite**: | ||
+ Finalized wording inside Specification document | ||
+ Proposed spec changes don’t have any blind spots (not described edge-cases, missed changes to related part of spec, etc.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not described → undescribed
stages.md
Outdated
+ Finalized wording inside Specification document | ||
+ Proposed spec changes don’t have any blind spots (not described edge-cases, missed changes to related part of spec, etc.) | ||
|
||
**Steps**: start working on “graphql-js” PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is the first mention of graphql-js in the document, should probably link to the repo.
32ddc19
to
607bc00
Compare
@wincent Thank you for proofreading 👍
I've added an one based on the text from your commit. What do you think?
I think Lee's list from GraphQL Europe talk can serve as a basis. So in theory, it will stimulate PR author to answer basic questions before making PR and thus save reviewers time and improve quality of PRs. But before adding something concrete I want to make small research and see how it's done in other WG, committee, etc. I added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the one thing that is missing here is a framing statement at the top to provide a little more context
I've added an one based on the text from your commit. What do you think?
Looks good!
stages.md
Outdated
+ Encourage community discussion around the proposed changes. | ||
|
||
Any changes to a specification that affect behavior of **GraphQL server or | ||
client implementation** should go through bellow stages no matter how small |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bellow → below
stages.md
Outdated
it is. All other changes should be labeled as “editorial” and could be merged | ||
right away. | ||
|
||
## Stage -2: proposed change(optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want whitespace before "(optional)" here, right? Same for the next stage heading as well.
Idea: perhaps this file should be named |
@wincent Great idea 💡 |
So, with this we have a draft definition of "stages for changes to GraphQL specification". What we don't have is a definition of "stages for changes to the definition of stages for changes to the GraphQL specification". My inclination is to merge this as-is (but add a "Draft" label) and iterate from there. |
@wincent I suggest we proceed toward
What do you think? |
I'd say we can afford to move a little more quickly/scrappily on merging a change like this: it is not a change to the spec itself, it requires no implementation work from library or product developers, it is easily revised once landed, and you've already canvassed the idea in the last (first?) WG meeting to broad (informal) approval. I wouldn't want something like this PR to get bogged down too heavily in process, especially when we can make the non-final nature of the document clear by labeling it as a draft (and at that point I think it would make sense to discuss it in detail at the next WG meeting). |
@wincent Make sense 👍 |
Yeah, that's almost exactly what I was thinking.
(Blockquote to make it stand out a bit.) |
@wincent ✅ Done |
- Capitalize "GraphQL Specification" (elsewhere in context it is informally referred to as "spec" or "specification"). - Use article "the" when referring to the spec. - Various minor copy edits for grammar. - Blank line after headings for source readablility. - Add link to checklist. - Use punctuation consistently (eg. trailing periods on list items). - Consistent capitalization.
Ok, great. This is a good starting point. We can iterate from here. Thanks @IvanGoncharov. |
Added discussion of this to the agenda for the next meeting: graphql/graphql-wg#22 |
This PR is a follow-up WG discussion about defining stages for changes to GraphQL specification.
I copied stage descriptions from my slides as an early draft to keep momentum and continue the conversation.