Skip to content

__Type represents all types in the system #775

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

Merged
merged 6 commits into from
Apr 8, 2021

Conversation

benjie
Copy link
Member

@benjie benjie commented Sep 4, 2020

In #733 (comment) @leebyron noted:

maybe we should just say: "It represents all types in the system."

I wasn't quite comfortable with what "all types" meant - whether it was inclusive or exclusive of modified types. The second paragraph implied exclusive due to the also but this didn't sit quite right with me. At first I edited it to say "all named types in the system", but felt it made the second paragraph feel a bit disjoint.

I've further modified the text and feel it's much clearer now.

I'm trying to move discussions like this out of the Tagged type spec itself, to keep the signal to noise ratio high (and figure they're useful mods for any future type additions anyway, keeping maintenance burden down).

@benjie benjie changed the title Type all types __Type represents all types in the system Sep 4, 2020
@benjie
Copy link
Member Author

benjie commented Sep 4, 2020

Incidentally we consistently refer to "type modifier" not as the concept of List/NonNull itself, but as a specific instance of these concepts applied to a specific type; I've tried to maintain this in this PR. I'd prefer to call List/NonNull themselves "type modifiers", and List(NamedType) a "modified type" - if this seems reasonable I'm happy to make the relevant edits.

@benjie
Copy link
Member Author

benjie commented Jan 21, 2021

@leebyron Could I get your eyes on this?

@leebyron leebyron added the ✏️ Editorial PR is non-normative or does not influence implementation label Jan 21, 2021
@leebyron
Copy link
Collaborator

Thanks for the reminder. I tagged it so I don't lose it :)

Let me give this a closer read when I get a free moment

Base automatically changed from master to main February 3, 2021 04:50
@benjie
Copy link
Member Author

benjie commented Mar 27, 2021

@leebyron Gentle nudge 😉

@benjie
Copy link
Member Author

benjie commented Apr 2, 2021

LGTM @leebyron 👍

@leebyron leebyron added this to the May2021 milestone Apr 6, 2021
@leebyron leebyron merged commit 792671b into graphql:main Apr 8, 2021
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.

2 participants