-
-
Notifications
You must be signed in to change notification settings - Fork 733
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
ICU-22747 MF2: Validate values of integer-valued options for :number and :integer #2973
base: maint/maint-75
Are you sure you want to change the base?
ICU-22747 MF2: Validate values of integer-valued options for :number and :integer #2973
Conversation
…and :integer Previously, it wasn't an error to write a message like:: .local $foo = {1 :integer minimumIntegerDigits=foo} {{$foo}} This is an error according to the spec, because the `minimumIntegerDigits` option to `:integer` must have a value that's a non-negative integer. This change adds validation of options that must be integer-valued to the implementations of the built-in `:integer` and `:number` functions. Other options (most of which have small sets of string values that are allowed as valid options) are not validated yet.
It is not an error to write a message like that, from the point of view that this is a "valid" message. I carefully searched the formatting and registry sections and double-checked errors. It is not currently required to be an error (as an alternative, an implementation might ignore the option, although I'm dubious that this is a Good Idea). The registry part of the spec says what the value space for digit options is, but not what to do if the value provided doesn't match it. (It also fails to clearly state that what to do is implementation defined). It is permitted that implementations emit errors during function resolution (in which case, this will be an Invalid Expression error). When this is addressed (and I think it should be) I think it should be addressed in default registry, not the core spec. |
I found these errors while [commenting](unicode-org/icu#2973 (comment)) on an issue in ICU. Fixing the text for consistency.
In general the philosophy in how errors are handled in JS / Python and other other can be quite different from something like ICU. If I do throw in ICU (C++ or Java) on something like a watch, it means a firmware update that has to be explicitly pushed to millions of devices. If the spec explicitly REQUIRES such mistakes to trigger errors, then I would strongly push to change the spec. |
@aphillips @mihnita When I say "the spec requires it", I mean in the sense that the tests are part of the spec. I think I raised the question before of whether the test should be required to be an error, and Eemeli's comment here said yes, which is what I'm going off. But I'm happy to close the PR (for now) if there needs to be more discussion on the spec side. |
@catamorphism Thanks for this. @eemeli's comment and what the tests do might be the "right thing to do", but, as noted, they are not what the spec says. We need to be very careful about creating conformance tests. They need to match what the spec actually requires. I think it is valid for an implementation (ICU4C is one) to emit an error for this and render the fallback expression. But these are not syntax or data model errors. |
I found these errors while [commenting](unicode-org/icu#2973 (comment)) on an issue in ICU. Fixing the text for consistency.
I'm converting this PR to draft since it's an ongoing topic of discussion in the spec working group. |
Previously, it wasn't an error to write a message like::
.local $foo = {1 :integer minimumIntegerDigits=foo} {{$foo}}
This is an error according to the spec, because the
minimumIntegerDigits
option to:integer
must have a value that's a non-negative integer.This change adds validation of options that must be integer-valued to the implementations of the built-in
:integer
and:number
functions. Other options (most of which have small sets of string values that are allowed as valid options) are not validated yet.Checklist