-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this 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.
There was a problem hiding this 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 👍
@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 👍 |
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
<#1078 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAMHUXEXI4CJAJERX75E6LYTS7Z7AVCNFSM6AAAAABDGNLD66VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBTHAZTAMZSHE>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
|
There was a problem hiding this 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
Co-authored-by: Benjie <benjie@jemjie.com>
420b969
to
045a347
Compare
As suggested in yesterdays WG this has been replaced with a note about composite |
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:
Don't want to bikeshed this too much though so either is fine by me 👍 |
@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 |
@benjie the force push was just to update this branch with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Co-authored-by: Benjie <benjie@jemjie.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this 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!
Co-authored-by: Benjie <benjie@jemjie.com>
There was a problem hiding this 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 👍
Co-authored-by: Benjie <benjie@jemjie.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to have a word to use for "Types that are schema-defined vertex types in your graph" but Composite isn't a great word for that, agree.
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