-
Notifications
You must be signed in to change notification settings - Fork 76
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
Improve Terminology? - nullable primitive type #203
base: main
Are you sure you want to change the base?
Conversation
@bgribaudo : Thanks for your contribution! The author(s) have been notified to review your proposed change. |
Learn Build status updates of commit 32dbf5f: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Can you review the proposed changes? When the changes are ready for publication, add a #label:"aq-pr-triaged" |
bgribaudo I suspect you are right about how the terminology was being used here, and I agree that this should be rephrased. I don't have a strong opinion on what we should change it to. The simplest might be to just say "primitive and nullable primitive types". I am also thinking of "optionally nullable primitive types", but this seems like it could still be confusing. |
Thanks, @egorelik93! What about calling these "non-custom types"? This would correspond nicely with (e.g. it would be the exact inverse of) what the spec calls custom types: "All types that are not members of the closed set of primitive types plus their nullable counterparts are collectively referred to as custom types." It seems like it would read clearly. Example:
|
Another idea could be the name "simple types" (vs. "complex types"—i.e. the types that require (), [] or {} in their definition). |
@egorelik93, hope your day is going well! Did you have any thoughts on this? |
@bgribaudo The way other systems use the term 'complex types', it would include things like 'record' and 'list', so I don't think I want to go there. I was trying to figure out what is the opposite of 'custom', and I think I have my answer: 'built-in types'. That being said, while I like that phrasing, it's not a phrase we've ever used before. Not that 'non-custom types' is either, nor does it really flow on the tongue, but since 'custom types' is documented, I don't think it needs any explanation. On the other hand, I'd have no issue introducing a new term 'built-in types' either. @ehrenMSFT @DougKlopfenstein any thoughts? |
@bgribaudo I take that back. 'Built-in types' could confuse people into thinking it include things like Int64.Type, which we don't want to go into. If 'primitive and nullable primitive' is too wordy, maybe let's just use 'non-custom' types. |
I think "primitive and nullable primitive" would be fine. It seems like that's the clearest option. I'd also be fine with just saying "primitive", since most people would probably assume that includes the nullable versions of those types as well. But if you want to be explicit and say "primitive and nullable primitive", I'm fine with that. |
Thanks, @egorelik93 and @ehrenMSFT! To me, a downside to just calling these types "primitive" is that that the term "primitive type" is given a specific meaning in the specification separate from the idea of nullable. Conflating the two could be confusing. "Primitive and nullable primitive" covers it, but is a little wordy.
@ehrenMSFT, are you strongly opposed to "non-custom types?" :-) The spec currently defines a subset of types as "custom types." What we are talking about here is the opposite/alternate to that subset, so arguably the prefix "non-" covers what we need it to. |
I'm fine with either "primitive and nullable primitive" or "non-custom". |
Learn Build status updates of commit 5aae8fa: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Learn Build status updates of commit a4149bb: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
@bgribaudo, @ehrenMSFT, @egorelik93 - Is this pull request ready to be merged? |
I'd recommend waiting for @egorelik93 to sign off. |
@bgribaudo I'm leaning back towards "primitive and nullable primitive". "non-custom" is fine for spec wizards but in isolation would be non-obvious. |
Wouldn't someone have to be a bit familiar with the spec to know what the term "primitive" means, as well? |
Learn Build status updates of commit 68a912d: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
@ehrenMSFT, @egorelik93, what do you think of this? :-) |
Learn Build status updates of commit 7d5d05f: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Hi @egorelik93 and @DougKlopfenstein! Happy New Year! Do you know the status of this PR? Thanks! |
Learn Build status updates of commit da75a6b: ❌ Validation status: errorsPlease follow instructions here which may help to resolve issue.
For more details, please refer to the build report. Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
Learn Build status updates of commit 4e9de2f: ❌ Validation status: errorsPlease follow instructions here which may help to resolve issue.
For more details, please refer to the build report. Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
@egorelik93, thanks for the feedback. Any better now? |
Learn Build status updates of commit 28ccf8c: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
|
||
The expression `x is y` returns `true` if the ascribed type of `x` is compatible with `y`, and returns `false` if the ascribed type of `x` is incompatible with `y`. `y` must be a _nullable-primitivetype_. | ||
The expression `x is y` returns `true` if the ascribed type of `x` is compatible with `y`, and returns `false` if it is not compatible. `y` must be a _primitive-or-nullable-primitive-type_. |
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.
Still some prose references to 'primtive-or-nullable-primitive-type' around here.
_primitive-or-nullable-primitive-type:_<br/> | ||
`nullable`_<sub>opt</sub> primitive-type_<br /> | ||
|
||
All other types are collectively referred to as _custom types_. Custom types can be written using a `type-expression`: |
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.
Is this supposed to be here? Seems redundant with the previous prose paragraph.
Background:
Currently,
Type.Is
's documentation gives no indication that its second argument should only be set to certain type values. A recent PR attempted to rectify this by updatingType.Is
's docs, copying over and expounding on the rule given in the spec which states thatType.Is
only accepts a "nullable primitive type value as its second argument."Problem:
However, when reviewing the PR, @ehrenMSFT pointed out that
Type.Is
works fine with second argument values such astype number
, which clearly is not nullable (e.g.Type.IsNullable(type number) // returns false
).So, the spec says that
Type.Is
's second argument must be a nullable primitive type, yet the function works fine with primitive type values that are not nullable.I think the reason for this dichotomy is in the definition of terms:
type number
).Type.IsNullable
test (so would excludetype number
).Question:
To avoid/resolve this confusion, should a different term be used in the spec for the set "all primitive types + their nullable counterparts"?
(Note: The term nullable primitive type is used a number of places in the spec, including in the details of the
as
andis
operators as well as in conjunction with the definition of function parameters. So, this change would affect a number of places in the spec.)