graphql-spec
graphql-spec copied to clipboard
Editorial: Clarify type modifiers text
This PR aims to improve the last minute changes to https://github.com/graphql/graphql-spec/pull/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.
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
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"
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.
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.
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.
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
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
ofTypeof 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?
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 :wink:
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: https://github.com/graphql/graphql-spec/issues/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?
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.