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.

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

Conversation

benjie
Copy link
Member

@benjie benjie commented Apr 8, 2021

This PR aims to improve the last minute changes to #775 by improving the readability of the paragraph. The text This modified type may recursively be a modified type is hard to grok, and we've referred to modified type twice here, whereas I believe we actually mean This modified type may recursively be a type modifier. Instead of making this small edit, I've tried to improve readability further by implicitly defining the "inner type" term that's used elsewhere in the spec (see #831 for context on this) and then using this to refer to the modified type.

@benjie benjie added the ✏️ Editorial PR is non-normative or does not influence implementation label Apr 8, 2021
@benjie benjie requested a review from leebyron April 8, 2021 07:59
@leebyron
Copy link
Collaborator

leebyron commented Apr 8, 2021

The last minute changes to #775

I'm sorry about that! My intent was to factor these out from #777 and land and rebase, but this one got additional editorial - should have waited for review. I really appreciate the careful read and this follow up

@leebyron
Copy link
Collaborator

leebyron commented Apr 8, 2021

This definitely clarifies, but maybe we need to back up and define some terms to expand this.

We use the terms "inner type" and "wrapped type", and "unwrapped type" interchangeably and should unify this term.

There are also some places in Section 3 where we call List and Non-Null "wrapping types" https://spec.graphql.org/draft/#sec-Wrapping-Types

Both wrapped and modified aren't ideal in that when standing alone they are ambiguous as to whether they refer to "the type being modified/wrapped" or "the resulting modified/wrapped type" - eg are they the input or output to the type modifier? Both "inner" and "unwrapped" avoid this ambiguity.

In Section 3 (Type System) we largely use the term "wrap" and in Section 4 (Introspection) use "modifier".

While in isolation I prefer the term "type modifier" I'm considering that "wrap" may be the better choice here because the three concepts share name form and we use it more across the existing spec.

Proposal, thoughts?

Wrapping Type

Described here: https://spec.graphql.org/draft/#sec-Wrapping-Types ie. List and Non-Null

We would replace references to "type modifier" and "modifying type" with "wrapping type"

Wrapped Type

A specific instance of a wrapping type applied to some type. ie. String[] and String!

We would replace references to "modified type" with "wrapped type"

Unwrapped Type

In the context of a wrapped type, the unwrapped type is the type contained within that wrapped type. ie. for String[] that is String and for String![]! that is String![]

We would replace references to "inner type" with "unwrapped type"

@andimarek
Copy link
Contributor

I think "inner type" is not so great as it sounds like just one level (but maybe this just me).
GraphQL Java and also GraphQL.js iirc uses the "wrapping" terminology which I think is good.

@leebyron leebyron added this to the May2021 milestone Apr 9, 2021
@benjie
Copy link
Member Author

benjie commented Apr 9, 2021

As commented in #831 I don’t like “unwrapped type” because it sounds like it’s removed ALL the layers of wrapping, not just one. If you were playing pass the parcel (I wonder how well that translates internationally...) the parcel would be unwrapped at the end of the game; during each turn you’d explicitly say “unwrap just one layer.” As @andimarek notes, “inner type” generally implies just one layer, which is what we want in pretty much all places I analysed in #831.

“Named type” can be used to refer to the deep unwrapped type.

“Wrapped type” is super ambiguous (thanks English!) as it could be used to reference the resulting type which wraps another (as you have suggested), or to reference the type that was wrapped (the inner type). My default interpretation is the latter. Same issue applies to “modified type”.

This seems to be challenging to solve non-ambiguously (like “bi-weekly”; thanks English!). Maybe we can call List and NonNull “type wrappers” since their purpose is to wrap a type (but they are not themselves a type), and instances of them “wrapper types” since they are now types of the wrapper variety?

In summary I propose the three terms you suggest above are replaced by “type wrappers”, “wrapper types” and “inner type” respectively.

@leebyron
Copy link
Collaborator

Sorry for letting this one sit - I think this is one of our last meaty editorial changes before we're cut-ready.


I'd love if we can keep the name symmetry, but obviously clarity is most important.

“Named type” can be used to refer to the deep unwrapped type.

Totally agree, "named type" is the right term for this concept.

Maybe we can call List and NonNull “type wrappers” since their purpose is to wrap a type (but they are not themselves a type), and instances of them “wrapper types” since they are now types of the wrapper variety?

I find these terms ambiguous as well. Especially "wrapper types" - that reads to me as the thing doing the wrapping rather than the resulting wrapped instances.

Loosely held, I'd favor no change if possible. We currently use "wrapping types" to describe List and Non-Null. I don't see "type wrappers" as better, or at least better enough to make that change.

“Wrapped type” is super ambiguous as it could be used to reference the resulting type which wraps another, or to reference the type that was wrapped

Yeah, this one is the toughest to crack. My proposal above was "wrapped type" and "unwrapped type" to refer to these two concepts - and introduce the two alongside each other to avoid ambiguity. I see "wrapper type" as less clear than "wrapped type" for this concept.

@leebyron leebyron force-pushed the main branch 4 times, most recently from e5d241d to 6c81ed8 Compare April 23, 2021 19:15
@benjie
Copy link
Member Author

benjie commented Apr 25, 2021

Loosely held, I'd favor no change if possible

I very much dislike the phrase This modified type may recursively be a modified type; I think we should attempt to resolve this if we can.

I've now pushed a change that removes 'Type modifier' (aka "Wrapping type") from the spec entirely and it seems to have worked really well IMO.

So my proposal is now:

Wrapped type: a type that wraps another type, instance of List/Non-Null, e.g. Int!

  • list wrapped type: instance of List, e.g. [Int]
  • non-null wrapped type: instance of Non-Null, e.g. Int!

Inner type: the ofType of a "wrapped type" e.g. [Int!]! -> [Int!]

Named type: the fully unwrapped type, e.g. [Int!]! -> Int

@leebyron
Copy link
Collaborator

leebyron commented Apr 26, 2021

Wrapped type: a type that wraps another type, instance of List/Non-Null, e.g. Int!

  • list wrapped type: instance of List, e.g. [Int]
  • non-null wrapped type: instance of Non-Null, e.g. Int!

I like this - just to make sure I understand since you didn't explicitly say so - this would keep "wrapping type" to describe "List" and "Non-Null" but "wrapped type" for "[Int]" and "Int!" (and specifically "list wrapped type" and "non-null wrapped type" for each)?

Inner type: the ofType of a "wrapped type" e.g. [Int!]! -> [Int!]

Named type: the fully unwrapped type, e.g. [Int!]! -> Int

This sounds good to me. In this case "unwrap" or "unwrapping" can refer to the process by which you convert a wrapped type to an inner or named type. It's a verb not a noun - but when we need to disambiguate or make it clear that these two concepts are related to some wrapped type it can be an past participle, "unwrapped inner type" and "unwrapped named type" both make sense to me.

I very much dislike the phrase This modified type may recursively be a modified type; I think we should attempt to resolve this if we can.

Agreed - in fact I don't think this is accurate based on our existing definitions, since "modified type" is outside of that set. Given what we're proposing now, I think this phrase with swapped terms could be: The unwrapped inner type may recursively be a wrapped type?

@benjie
Copy link
Member Author

benjie commented Apr 27, 2021

just to make sure I understand since you didn't explicitly say so - this would keep "wrapping type" to describe "List" and "Non-Null" but "wrapped type" for "[Int]" and "Int!" (and specifically "list wrapped type" and "non-null wrapped type" for each)?

Yes; good catch. I've managed to make it so that we don't need this phrase at all in section 4, but we have Wrapping Types defined in section 3 and I think it makes sense to keep this phrase intact for now. I don't really like it (since they're not really "types" on their own, they're more "type factories" since you combine them with an existing ofType type to make a new type) but let's move on 😉

This sounds good to me. In this case "unwrap" or "unwrapping" can refer to the process by which you convert a wrapped type to an inner or named type.

Agree, with the caveat that it's ambiguous as to how much unwrapping you are doing (one layer, or all layers?).

"unwrapped inner type" and "unwrapped named type" both make sense to me.

I think this adds ambiguity: it could be taken as you say, but it could also be interpretted as taking the inner type and then unwrapping it further. I don't think unwrapped is needed in either of these cases - the inner type is the contents of the wrapped type by definition, and the named type may or may not have been wrapped in the first place. Rather than adding clarity, I feel adding unwrapped here adds confusion.

We use unwrapped nullable type and unwrapped item type in a few places in the spec; but I'd rather call these inner type (or inner nullable type and inner item type if extra clarity is required) - I plan to propose this more formally at some point, ref: #831.

I think this phrase with swapped terms could be: The unwrapped inner type may recursively be a wrapped type?

As noted, I don't think the word unwrapped here is necessary or beneficial; the current proposed text reads The inner type may recursively be a wrapped type which I think is sufficient?

@benjie
Copy link
Member Author

benjie commented Apr 29, 2021

Aside: I had a bit of a revelation that what we're currently calling "wrapped types" are really "wrapped type types" - i.e. the "wrapped type type" [Int] is a type that represents another type that it wraps (the "wrapped type") Int. Of course putting "wrapped type types" in the spec is... distasteful... but it does at least explain the ambiguity I perceive over whether "wrapped type" should refer to the inner type that was wrapped, or the type that was the result of wrapping the inner type.

I also stand by the statement that "wrapper types" (List, NonNull) are really "type wrappers" - they're not a type in-and-of themselves - and so we could also have "type wrapper types" to represent instances of these, but again this feels a bit distasteful.

That's enough lingual ambiguity discourse for me, I'm going to go back to the warm fuzzy certainty of code.

Comment on lines +232 to +233
Wrapped types modify the type presented in the field `ofType` (the "inner
type"). The inner type may recursively be a wrapped type, representing lists,
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.

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

@leebyron leebyron removed this from the May2021 milestone May 21, 2021
leebyron added a commit that referenced this pull request May 21, 2021
This broad change uses definition phrases to define each type, as well as expands the set of sub-sections at the top of the "Types" section which define *kinds* of types. In doing so applies the definitions arrived at in #845 for "wrapped types".

Makes editorial improvements throughout, removing duplicate language or attempting to improve correctness or legibility.
leebyron added a commit that referenced this pull request May 27, 2021
This broad change uses definition phrases to define each type, as well as expands the set of sub-sections at the top of the "Types" section which define *kinds* of types. In doing so applies the definitions arrived at in #845 for "wrapped types".

Makes editorial improvements throughout, removing duplicate language or attempting to improve correctness or legibility.
leebyron added a commit that referenced this pull request May 27, 2021
This broad change uses definition phrases to define each type, as well as expands the set of sub-sections at the top of the "Types" section which define *kinds* of types. In doing so applies the definitions arrived at in #845 for "wrapped types".

Makes editorial improvements throughout, removing duplicate language or attempting to improve correctness or legibility.
leebyron added a commit that referenced this pull request May 27, 2021
This broad change uses definition phrases to define each type, as well as expands the set of sub-sections at the top of the "Types" section which define *kinds* of types. In doing so applies the definitions arrived at in #845 for "wrapped types".

Makes editorial improvements throughout, removing duplicate language or attempting to improve correctness or legibility.
leebyron added a commit that referenced this pull request May 27, 2021
This broad change uses definition phrases to define each type, as well as expands the set of sub-sections at the top of the "Types" section which define *kinds* of types. In doing so applies the definitions arrived at in #845 for "wrapped types".

Makes editorial improvements throughout, removing duplicate language or attempting to improve correctness or legibility.
leebyron added a commit that referenced this pull request May 27, 2021
This broad change uses definition phrases to define each type, as well as expands the set of sub-sections at the top of the "Types" section which define *kinds* of types. In doing so applies the definitions arrived at in #845 for "wrapped types".

Makes editorial improvements throughout, removing duplicate language or attempting to improve correctness or legibility.
@benjie
Copy link
Member Author

benjie commented Sep 19, 2024

We now use the terminology "type modifiers" which feels better; I've not bothered to fully review if any of this is still useful, but it's 3 years old and I've got enough open PRs... I'm closing it.

@benjie benjie closed this Sep 19, 2024
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.

3 participants