graphql-spec icon indicating copy to clipboard operation
graphql-spec copied to clipboard

chore: add clarifying note for composite and expand term

Open JoviDeCroock opened this issue 1 year ago • 8 comments

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

JoviDeCroock avatar Feb 13 '24 12:02 JoviDeCroock

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.

netlify[bot] avatar Feb 13 '24 12:02 netlify[bot]

@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:

benjie avatar Feb 14 '24 14:02 benjie

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: @.***>

leebyron avatar Feb 14 '24 15:02 leebyron

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

JoviDeCroock avatar Mar 08 '24 07:03 JoviDeCroock

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 👍

martinbonnin avatar Mar 08 '24 10:03 martinbonnin

@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).

benjie avatar Mar 27 '24 11:03 benjie

@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 🙌

JoviDeCroock avatar Mar 27 '24 11:03 JoviDeCroock

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

benjie avatar Mar 27 '24 16:03 benjie