graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.13k forks source link

chore: add clarifying note for composite and expand term #1078

Closed JoviDeCroock closed 5 months ago

JoviDeCroock commented 9 months ago

Resolves https://github.com/graphql/graphql-spec/issues/1076

This PR adds a clarifying note for the term composite in prior versions of the spec and expands the term into Object, Interface or Union type wherever it was found

netlify[bot] commented 9 months ago

Deploy Preview for graphql-spec-draft ready!

Name Link
Latest commit 7222ffb4907c16fd006e847712622ef9b3099aec
Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/66044c49610d1a00084c70e4
Deploy Preview https://deploy-preview-1078--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.

benjie commented 9 months ago

@graphql/tsc Can we get another approval, then we'll wait a week or two for any more feedback and if no-one has concerns I think we should merge :+1:

leebyron commented 9 months ago

I’m OK with this change however I’m pretty sure the term composite type shows up all over the graphql-js repository as well

On Wed, Feb 14, 2024 at 6:03 AM Benjie @.***> wrote:

@graphql/tsc https://github.com/orgs/graphql/teams/tsc Can we get another approval, then we'll wait a week or two for any more feedback and if no-one has concerns I think we should merge 👍

— Reply to this email directly, view it on GitHub https://github.com/graphql/graphql-spec/pull/1078#issuecomment-1943830329, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAMHUXEXI4CJAJERX75E6LYTS7Z7AVCNFSM6AAAAABDGNLD66VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBTHAZTAMZSHE . You are receiving this because you are on a team that was mentioned.Message ID: @.***>

JoviDeCroock commented 8 months ago

As suggested in yesterdays WG this has been replaced with a note about composite

martinbonnin commented 8 months ago

I was thinking we would keep "composite" out of the normative sections (initial PR)?

And add a non-normative note that "composite" may be found in other contexts:

Note: implementers may use the term composite for a type that is either an object, interface
or union.

Don't want to bikeshed this too much though so either is fine by me 👍

benjie commented 8 months ago

@JoviDeCroock I suggest you undo your force-push and then add a note along the lines of what you just added; essentially the change should be to remove "composite" from most places (preferring explicitness) and then to add a non-normative note indicating that previous versions of the spec used and some implementers use the term "composite type" to refer to Object, Interface and Union types.

Also, the letter after Note: should be a capital since that is what is rendered in the note box (the Note: prefix is stripped out).

JoviDeCroock commented 8 months ago

@benjie the force push was just to update this branch with main I re-added the composite rephrasing and updated the note. Thank you for the review 🙌

benjie commented 8 months ago

I believe @Keweiqu and @leebyron's previous reviews are still valid with the latest state, though they may want to review the added note.