-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add a :math function, for addition and subtraction #932
base: main
Are you sure you want to change the base?
Conversation
@@ -381,6 +381,69 @@ together with the resolved options' values. | |||
|
|||
The _function_ `:integer` performs selection as described in [Number Selection](#number-selection) below. | |||
|
|||
### The `:math` function | |||
|
|||
The function `:math` is a selector and formatter for matching or formatting |
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 a formatting function, is it? A formatting function would require all of :number
or :integer
's options, e.g. to control zero digits, separators, and the like.
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.
As proposed, you would be able to format a :math
annotated value, so doesn't that make it a formatting function? If you need the options, you can set them in a separate declaration.
- ([digit size option](#digit-size-options)) | ||
|
||
If no options or more than one option is set, | ||
or if an _option_ value is not a [digit size option](#digit-size-options), |
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 not a => does not resolve to?
we want to allow {$n :math subract=$x}
where $x contains a digit size option literal
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.
Yes; that's covered by this in the digit size option definition:
Implementations MAY also accept implementation-defined types as the value.
|
||
#### Selection | ||
|
||
The _function_ `:math` performs selection as described in [Number Selection](#number-selection) below. |
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.
Should this make it clear that the selection is done after the math is performed?
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.
We could, but it's not technically necessary, as Number Selection includes this:
When implementing
MatchSelectorKeys(resolvedSelector, keys)
whereresolvedSelector
is the resolved value of a selector andkeys
is a list of strings, numeric selectors perform as described below.
which ends up referring back to the preceding Resolved Value section, which clarifies when the modifications are done on the resolved value.
a _Bad Option_ error is emitted | ||
and a _fallback value_ used as the _resolved value_ of the _expression_. | ||
|
||
#### Resolved Value |
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.
Do we need a note about implementation specific boundary conditions, e.g.
{$n :math add=$maxIntegerOnMyPlatform}
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 don't think so; that's also covered under digit size option:
Implementations MAY define an upper limit on the resolved value of a digit size option option consistent with that implementation's practical limits.
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.
Right, although you can get problems with overflow without approaching the "practical limits" for the option value, e.g.
{2147483646 :math add=2} // exceeds Java int by 1
I think a note is called for. I'll propose one when I do a full review.
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.
Tbf, an integer overflow does seem like a practical limit to me.
Co-authored-by: Addison Phillips <addison@unicode.org>
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.
Whoa, I must of missed this earlier. It is seriously wrong.
#### Selection | ||
|
||
The _function_ `:math` performs selection as described in [Number Selection](#number-selection) below. | ||
|
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 see some significant problems in these formulations.
The resolved value of an expression with a
:number
function
contains an implementation-defined numerical value
of the operand of the annotated expression,
together with the resolved options' values.
If that is all the resolved value is, then there is no difference in resolved value between the following two.
.local $x1 = {$x :integer}
.local $x2 = {$x :number}
But of course there is a big difference: the resolved value of $x1 contains a :integer annotation and the resolved value of $x2 contains a :number annotation, as in:
such as the _resolved value_ of an _expression_ with a `:number` or `:integer` _annotation_,
And it has to be that way, otherwise no .local setting later on could tell the difference between them.
So I think L142 of formatting.md is insufficient:
interface MessageValue { formatToString(): string formatToX(): X // where X is an implementation-defined type getValue(): unknown resolvedOptions(): { [key: string]: MessageValue } selectKeys(keys: string[]): string[] }
It also needs do have something like: getFunctionAnnotation();
In shorthand, the resolved value needs to logically contain a triple {datatype, functionAnnotation, optionMap}. Otherwise, a subsequent .local statement using a resolved value wouldn't have enough information to know how to compose.
So let's look at the following:
The _operand_ of a number function is either an implementation-defined type or
a literal whose contents match the `number-literal` production in the [ABNF](/spec/message.abnf).
All other values produce a _Bad Operand_ error.
But the operand can also be a resolved value, which is neither a literal nor just an implementation datatype; it has additional information.
And in multiple cases in this file, we have wording like:
Resolved Value
The resolved value of an expression with a :number function contains an implementation-defined numerical value of the operand of the annotated expression, together with the resolved options' values.
Now, if somewhere we say that the resolved value of any expression with a function contains that function as an annotation, then we could get away with this shorthand (although we also need to fix the wording for options).
Otherwise, we need to say:
The resolved value of an expression with a :number function contains an implementation-defined numerical value of the operand of the annotated expression, together with the :number function annotation, and the options with their respective resolved option~'s~ values.
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 don't get it.
The composition model is that functions can expose specific options, but the identity of the function itself is not passed. There is no difference between the resolved values of {$x :number}
and {$x :integer}
with regard to the receiving function: it's just a numeric type. A subsequent local declaration doesn't need to know which annotation was previously applied: the preceding function handler modified the resolved value and/or passed the relevant options to communicate what happened.
It has to be that way, because otherwise each function would need to understand what all of the other functions (including namespaced ones) "mean".
If {$x :math subtract=2}
doesn't return $x - 2
as its resolved value, :number
isn't going to know what to do.
There are places where I have actively resisted "doing harm" to the resolved value based on the annotation. That's an argument we can have. Mostly I'm concerned that we don't strip information from the resolved value because of some formatting instructions:
.input {$now :date style=medium}
.local $time = {$now :time style=short}
{{"Today is {$now} and it is {$time}" only works if :date doesn't remove the time.
I assume that is okay because immutability says that if $now is a Date object or ZonedDateTime
there is no need to convert it to just a ZonedDate and make the time midnight.}}
We could change :integer
to make the resolved value the floor value of the operand or to make it the implementation-defined integer-type casting of the operand. That doesn't break immutability because the resolved value is only exposed in a declaration. I think that's the better course than what you're proposing.
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 think I conflated too many thoughts in my message. So put on hold the notion of the function annotation (not needed for release) and I focus on just 2 issues.
- The operand for a function may also be a resolved value (which includes an optionMap). That is not the same as an implementation defined type (eg Date); a resolved value can have an implementation-defined type. Thus
OLD
The operand of a number function is either an implementation-defined type or
a literal whose contents match thenumber-literal
production in the ABNF.
All other values produce a Bad Operand error.
needs to be changed to:
NEW
The operand of a number function is either an implementation-defined type or
a literal whose contents match thenumber-literal
production in the ABNF,
or a resolved value.
All other values produce a Bad Operand error.
(with similar changes for other definitions of operand of an X function.)
- The expression "together with the resolved options' values." is not accurate. For {|3| :number roundingIncrement=5 trailingZeroDisplay=auto}, that would imply a logical result of
{operand=3, resolvedOptionValues=[5, auto]}
rather than the logical result of
{operand=3, resolvedOptionMap={roundingIncrement=5, trailingZeroDisplay=auto}}
So we need to have:
OLD
... value of the operand of the annotated expression, together with the resolved options' values.
changed to something like:
NEW
... value of the operand of the annotated expression, together with a map from options to their resolved values
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.
The problem with the first item is that the resolved value might not be of the correct implementation-defined type. The current text accounts for this by not mentioning. We could say instead:
The operand of a number function is either an implementation-defined type or
a literal whose contents match thenumber-literal
production in the ABNF
(either of which can be the resolved value of a previous declaration).
All other values produce a Bad Operand error.
The second suggestion we already discussed. Saying "map" was thought too prescriptive (ditto "array", "list", etc.). I can see how one might read "options' values" to mean only the values and not the k-v pairing. We might say:
... value of the operand of the annotated expression, together with options and their resolved values.
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.
One difference between {$x :number}
and {$x :integer}
is that the resolved value of the former holds a numeric value, while the resolved value of the latter holds an integer value. Given that :integer
also strips out a pre-defined set of options if they're included in the operand, and we don't mandate that the actual type of the number values match, I don't see a reason why anything in MF2 requires the resolved values to be more identifiable from each other?
The operand for a function may also be a resolved value (which includes an optionMap). That is not the same as an implementation defined type (eg Date); a resolved value can have an implementation-defined type.
The intent with the current language is that a resolved value can be considered as "an implementation defined type". We do not require an implementation to only define a single type that it supports, so both a Date
and the resolved value of a :datetime
annotation can simultaneously fit into that definition.
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.
From Addison
The problem with the first item is that the resolved value might not be of the correct implementation-defined type. The current text accounts for this by not mentioning. We could say instead:
The operand of a number function is either an implementation-defined type or
a literal whose contents match thenumber-literal
production in the ABNF
(either of which can be the resolved value of a previous declaration).
All other values produce a Bad Operand error.
Wouldn't quite work. Again, we have defined the resolved value of {|3| :number style=percent} to be NOT the same as |3| — since the resolved value includes options. I think what we could say:
(either of which can be from the resolved value of a previous declaration).
I think that is at least headed in the right direction, though not as correct as
or a resolved value with those as an operand.
The second suggestion we already discussed. Saying "map" was thought too prescriptive (ditto "array", "list", etc.). I can see how one might read "options' values" to mean only the values and not the k-v pairing. We might say:
... value of the operand of the annotated expression, together with options and their resolved values.
That sounds perfect.
I really don't see how this is cleaner than the It looks very close to a Which seem cleaner as implementation, and logic, but less intuitive for a user:
As a user is not intuitive at all why would I select on |
Fixes #701.
An alternative to #926.
Adds
:math
as a new numeric function that canadd
orsubtract
from the operand.Note that the option values are more strict than for other functions; option values beyond "digit size option" are considered errors.