Skip to content
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

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

JoviDeCroock
Copy link

@JoviDeCroock JoviDeCroock commented Feb 13, 2024

Resolves #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

Copy link

netlify bot commented Feb 13, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 7222ffb
🔍 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.

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 support the replacement of "composite types" in section 5 with more explicit language.

spec/Section 5 -- Validation.md Outdated Show resolved Hide resolved
spec/Section 4 -- Introspection.md Outdated Show resolved Hide resolved
spec/Section 5 -- Validation.md Outdated Show resolved Hide resolved
@benjie benjie added the ✏️ Editorial PR is non-normative or does not influence implementation label Feb 14, 2024
benjie
benjie previously approved these changes Feb 14, 2024
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 this helps clarity 👍

@benjie
Copy link
Member

benjie commented Feb 14, 2024

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

@leebyron
Copy link
Collaborator

leebyron commented Feb 14, 2024 via email

Copy link
Contributor

@Keweiqu Keweiqu left a comment

Choose a reason for hiding this comment

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

Makes sense! Thanks for doing this

JoviDeCroock and others added 2 commits March 8, 2024 08:06
@JoviDeCroock
Copy link
Author

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

@martinbonnin
Copy link
Contributor

martinbonnin commented Mar 8, 2024

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
Copy link
Member

benjie commented Mar 27, 2024

@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 JoviDeCroock changed the title chore: remove mentions of composite chore: add clarifying note for composite Mar 27, 2024
@JoviDeCroock
Copy link
Author

@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 JoviDeCroock changed the title chore: add clarifying note for composite chore: add clarifying note for composite and expand term Mar 27, 2024
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.

Looking good!

spec/Section 5 -- Validation.md Outdated Show resolved Hide resolved
Co-authored-by: Benjie <benjie@jemjie.com>
Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

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

👍

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.

This one got undone accidentally I think!

spec/Section 5 -- Validation.md Outdated Show resolved Hide resolved
Co-authored-by: Benjie <benjie@jemjie.com>
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.

Looks great to me 👍

@benjie
Copy link
Member

benjie commented Mar 27, 2024

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

Co-authored-by: Benjie <benjie@jemjie.com>
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.

Composite types is not clearly defined
6 participants