Skip to content

Fix: Leaf Field Selections for enums must also be empty #452

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

Merged
merged 3 commits into from
Jun 11, 2018

Conversation

matprec
Copy link
Contributor

@matprec matprec commented May 29, 2018

No description provided.

@IvanGoncharov
Copy link
Member

@MSleepyPanda Thanks for quick PR 👍
Would be great if you also fix Explanatory Text in the same section, to keep it in sync.

@matprec
Copy link
Contributor Author

matprec commented May 29, 2018

While i'm at it, re Example 114:
fragment scalarSelectionsNotAllowedOnBoolean on Dog: Why Boolean? barkVolume is an Int. Also the intent is to convey that no further selections are allowed on primitives.

I'd propose changing the name to scalarSelectionsNotAllowed. I'd expand the example by enumSelectionsNotAllowed.

@IvanGoncharov
Copy link
Member

While i'm at it, re Example 114:
fragment scalarSelectionsNotAllowedOnBoolean on Dog: Why Boolean? barkVolume is an Int. Also the intent is to convey that no further selections are allowed on primitives.

@MSleepyPanda Good catch 👍

I'd propose changing the name to scalarSelectionsNotAllowed. I'd expand the example by enumSelectionsNotAllowed.

Makes total sense to me, but I suggest keeping onInt part to prevent possible confusion.

@@ -568,7 +568,7 @@ fragment conflictingDifferingResponses on Pet {

**Explanatory Text**

Field selections on scalars are never allowed: scalars
Field selections on scalars or enums are never allowed: scalars and enums
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe instead of : scalars and enums write because they are the leaf nodes of any GraphQL query.

@IvanGoncharov
Copy link
Member

@MSleepyPanda This and #451 both look good 👍
Next step is the review by @leebyron

If you find any other places where clarification needed to feel free to open PR or create an issue.

@leebyron leebyron merged commit 87a0d80 into graphql:master Jun 11, 2018
@leebyron
Copy link
Collaborator

Great edit, thank you!

@leebyron leebyron added ✏️ Editorial PR is non-normative or does not influence implementation and removed clarification labels Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editorial PR is non-normative or does not influence implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants