Skip to content

Editorial: Clarify type modifiers text #845

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Editorial: Clarify type modifiers text #845

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
68a1744
Clarify type modifiers text
benjie Apr 8, 2021
b2532b1
Remove 'Type modifier' from the spec text
benjie Apr 25, 2021
e9ed944
Merge branch 'main' into type-modifiers-reword
benjie Apr 25, 2021
48220ae
This -> The
benjie Apr 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions spec/Section 4 -- Introspection.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,10 +227,10 @@ enum __DirectiveLocation {

`__Type` is at the core of the type introspection system, it represents all
types in the system: both named types (e.g. Scalars and Object types) and
type modifiers (e.g. List and Non-Null types).
wrapped types (e.g. List and Non-Null types).

Type modifiers are used to modify the type presented in the field `ofType`.
This modified type may recursively be a modified type, representing lists,
Wrapped types modify the type presented in the field `ofType` (the "inner
type"). The inner type may recursively be a wrapped type, representing lists,
Comment on lines +232 to +233
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we either need to rephrase this or change this term to "Wrapping types" since a "wrapped type" is modified while a "wrapping type" is modifying.

So either:

Wrapping types modify the type presented in the field ofType (the "inner type"). The inner type may recursively be a wrapped type, representing lists...

Or:

A Wrapped type represents lists and non-null of other types. It wraps the type presented in the field ofType (the "inner type"). The inner type may recursively be a wrapped type, representing lists...

Copy link
Member Author

Choose a reason for hiding this comment

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

Of these, the latter seems clearer to me.

non-nullables, and combinations thereof, ultimately modifying a named type.

### Type Kinds
Expand Down Expand Up @@ -352,11 +352,11 @@ Fields

#### List

Lists represent sequences of values in GraphQL. A List type is a type modifier:
Lists represent sequences of values in GraphQL. A List type is a wrapped type:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this phrasing is ambiguous/confusing. A list type is a "wrapping type" but an instance of a list type is a "wrapped type". Within introspection we only see instances, so this usage is probably right, but this phrasing relative to "A List type" makes this distinction a bit less clear IMO.

What do you think about:

Lists represent sequences of values in GraphQL. A List wrapped type expects a List value. It wraps another type in the ofType field (the "inner type"), which defines the type of each item in the list.

This replaces "A List type is a wrapped type" with "A List wrapped type" - using your definition

Copy link
Collaborator

Choose a reason for hiding this comment

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

Scratch this feedback. I'm working on a broader change based on yours that applies this new terminology broadly and I think I can resolve this at the definition of "List type"

Copy link
Member Author

Choose a reason for hiding this comment

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

This replaces "A List type is a wrapped type" with "A List wrapped type"

This reads a lot better 👍

Scratch this feedback. I'm working on a broader change based on yours that applies this new terminology broadly and I think I can resolve this at the definition of "List type"

Okay 👍

it wraps another type instance in the `ofType` field, which defines the type of
each item in the list.

The modified type in the `ofType` field may itself be a modified type, allowing
The type presented in the `ofType` field may itself be a wrapped type, allowing
the representation of Lists of Lists, or Lists of Non-Nulls.

Fields
Expand All @@ -370,13 +370,13 @@ Fields

GraphQL types are nullable. The value {null} is a valid response for field type.

A Non-Null type is a type modifier: it wraps another type instance in the
A Non-Null type is a wrapped type: it wraps another type instance in the
`ofType` field. Non-null types do not allow {null} as a response, and indicate
required inputs for arguments and input object fields.

The modified type in the `ofType` field may itself be a modified List type,
The type presented in the `ofType` field may itself be a List wrapped type,
allowing the representation of Non-Null of Lists. However it must not be a
modified Non-Null type to avoid a redundant Non-Null of Non-Null.
Non-Null wrapped type to avoid a redundant Non-Null of Non-Null.

* `kind` must return `__TypeKind.NON_NULL`.
* `ofType`: Any type except Non-Null.
Expand Down