Skip to content

add explanation about argument name uniquess. #739

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
wants to merge 1 commit into from
Closed

add explanation about argument name uniquess. #739

wants to merge 1 commit into from

Conversation

dugenkui03
Copy link
Contributor

Add explanation about argument name uniquess.

Copy link
Member

@benjie benjie 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 it would make sense to do these assertions in the same order as fields; i.e.

  1. Unique
  2. No __
  3. Type check

@dugenkui03
Copy link
Contributor Author

I think it would make sense to do these assertions in the same order as fields; i.e.

  1. Unique
  2. No __
  3. Type check

thanks for you suggestion, it does more readable.
the changes has been squashed to a single commit.

@IvanGoncharov IvanGoncharov added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Jun 25, 2020
@leebyron leebyron added 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) and removed 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) labels Jul 2, 2020
@leebyron
Copy link
Collaborator

leebyron commented Jul 2, 2020

Marking this proposal as Stage 1.

Next steps are making sure this has a reflecting PR to the graphql.js reference implementation

Base automatically changed from master to main February 3, 2021 04:50
@leebyron leebyron force-pushed the main branch 4 times, most recently from e5d241d to 6c81ed8 Compare April 23, 2021 19:15
@dugenkui03
Copy link
Contributor Author

Hi @leebyron @IvanGoncharov,would you be willing to merge this pr directly, or this process must finish in stage 3.

I think it's a pretty simple supplementary explanation: the graphql implementation of various languages must obey it, but specification haven't written it directly.

In fact, we can infer these validation rules from the specification.

@IvanGoncharov
Copy link
Member

would you be willing to merge this pr directly, or this process must finish in stage 3.

@dugenkui03 We need to go full process.
Can you please do git --amend + git push to trigger our new CI?
That way we can be sure you signed CLA and is able to merge your PR once it reaches stage3.

I actually forgot about this PR so when working on graphql/graphql-wg#505 I independently did the same changes in #877
Happy to close #877 in favor of this once we are sure that you sign CLA and we can merge your PR.

@netlify
Copy link

netlify bot commented Oct 7, 2021

❌ Deploy Preview for graphql-spec-draft failed.

🔨 Explore the source changes: 3fcc954

🔍 Inspect the deploy log: https://app.netlify.com/sites/graphql-spec-draft/deploys/615e6aead8f9fa00072207cb

@dugenkui03
Copy link
Contributor Author

would you be willing to merge this pr directly, or this process must finish in stage 3.

@dugenkui03 We need to go full process. Can you please do git --amend + git push to trigger our new CI? That way we can be sure you signed CLA and is able to merge your PR once it reaches stage3.

I actually forgot about this PR so when working on graphql/graphql-wg#505 I independently did the same changes in #877 Happy to close #877 in favor of this once we are sure that you sign CLA and we can merge your PR.

@IvanGoncharov Thanks for your response. After git push, some checks were not passed because of Deploy directory 'public' does not exist. Is it okay to create a new PR #891

@IvanGoncharov IvanGoncharov removed the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label Oct 7, 2021
@IvanGoncharov
Copy link
Member

@dugenkui03 Thanks, closing in favor of #891

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants