Skip to content

Conversation

@andimarek
Copy link
Contributor

Hey ... I hope that can be seen as editorial clarification:

In the overlapping field merging validation the term "pair" is used twice, without making it perfectly clear that the two pair members should be distinct.

In theory they don't have to be different, therefore this PR clarifies it.

Andi

@netlify
Copy link

netlify bot commented Feb 24, 2025

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 2b395c4
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/67bcfbc46e53cc0008510424
😎 Deploy Preview https://deploy-preview-1136--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@andimarek andimarek changed the title Clarify pair members are distinct Field merging validation: clarify pair members are distinct Feb 24, 2025
@benjie benjie added the ✏️ Editorial PR is non-normative or does not influence implementation label Feb 26, 2025
@andimarek
Copy link
Contributor Author

What was our process for editorial changes again? How long do we wait after getting some approvals? cc @benjie ?

@benjie
Copy link
Member

benjie commented Feb 26, 2025

I generally give it a couple weeks and then merge if it has approvals. (This is already in my queue.)

But this one is so trivial, we might as well go ahead and merge it.

@benjie
Copy link
Member

benjie commented Feb 26, 2025

I was just reading it again and I don’t think it really matters whether or not they’re distinct, since they will have the same response shape if they’re the same… But more efficient this way 🤷

@benjie benjie merged commit a1c025f into graphql:main Feb 26, 2025
9 checks passed
@github-project-automation github-project-automation bot moved this from SHIP IT to Done in Benjie's GraphQL tasks Feb 26, 2025
@andimarek
Copy link
Contributor Author

@benjie yes it is more performant this way, but maybe even a bit more important it makes it more clear why it should run on every SelectionSet instead of per Operation. If the fields don't need to be distinct you could run the whole validation once.

So overall this version I believe make more sense. Thanks for merging!

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.

3 participants