Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement
:unit
as Proposed RECOMMENDED in the registry #922base: main
Are you sure you want to change the base?
Implement
:unit
as Proposed RECOMMENDED in the registry #922Changes from all commits
f05d0ba
9a5b46a
4a623f9
aaba292
8786990
b5ef049
59ae657
8d83a8f
a76eedc
d0eb3e3
57582ee
0ce878b
8fe64c9
0c7beb2
b90a720
13e33f0
b5cf412
94c0dec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's not clear here that not all "implementation-defined types" are necessarily supported as operands.
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.
I'm not sure I understand. I don't think this needs to indicate that some types are not supported as operands.
This text is not about the operand. It's about the handling of option values within a resolved value. It is nearly boilerplate, since most of our functions have a variation of this sentence. In practice, it might be better to say something like the following, which doesn't care what the operand is or is not:
Such a boilerplate would be altered if some of the options were required to be filtered by the function.
unit
is candidate for this in:unit
(since theunit
presumably is now part of the resolved operand).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.
These don't really work right together. If unit conversion is applied, the formatted value can be completely different from the "unit value of the operand of the annotated 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.
Take 2 cases:
Suppose $n is an implementation-defined unit value, and the locale is en-US.
So what is happening in both cases is that the numeric value for selection is equivalent to what the formatted value has. And that is what needs to be selected on, namely 66.
The wording we have for that in selection as described in
So in both cases, the resolved value has to be 66.
There are of course other cases: n$ = {number=66.1387} and we format {$n :number maximumSignificantDigits=1} we'll get "70" (In English), and "٧٠" in Arabic. And that must match a selection key of 70.
So in this case, the resolved value has to be 70.
I think we haven't yet nailed 'resolved value', because the resolved value for selection and formatting ends up needing to be different than the resolved value for functional composition. But that we cannot 'resolve' at this point in the release.
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.
I understand that what you describe is likely the intent here, but the resolved value description talks of "an implementation-defined unit value of the operand of the annotated expression", with no reference to any conversion possibly being made to it, and so it's reasonable to assume that e.g. your case 1 example would end up with the numerical value
30
being the value that selection is done on, and as the selection is the same as used for other numerical values, it doesn't know about needing to multiply the number by any such factor.Note how in
:math
, the numeric value held in resolved value is explicitly modified by theadd
andsubtract
option values. We need something like that here.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.
If we permitted
unit
to convert, I agree that the resolved value should be modified. But I think we should do like:currency
and only allowunit
to compose with a numeric operand.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.
That works for me, and is safe and far simpler.
It would still be possible for a future version to allow that, but I think one would have to make a good case for doing so.
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.
According to the text above, unit conversion is also possible via the
unit
option. That should be mentioned here beyond its errant use in the example below, possibly along with some description of what happens if you define bothunit
andusage
.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.
Agreed.