Skip to content
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

Adding a :number offset option #926

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mihnita
Copy link
Collaborator

@mihnita mihnita commented Nov 7, 2024

Fix for issue #701

@mihnita
Copy link
Collaborator Author

mihnita commented Nov 7, 2024

TODO: add tests.

But I wanted to unblock the splitting of the registry in separate files.
Plus, the wording is the the part that might be controversial.
The tests, not so much.

@mihnita mihnita added registry LDML46.1 MF2.0 Draft Candidate labels Nov 7, 2024
@mihnita mihnita changed the title Adding a :number offset option (#701) Adding a :number offset option Nov 7, 2024
Copy link
Collaborator

@eemeli eemeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the option is available on :number, it should also be on :integer.

OTOH, if it only supports whole number values, it might make sense to only allow on :integer; I'm not aware of any use case for it having been presented for non-integer values.

I also submitted #927 as the text around resolved value should be clarified.

Comment on lines +208 to +209
The _resolved value_ of the _expression_ is determined by first subtracting the numeric value of `offset` from the _operand_,
and then resolving the _expression_ on the calculated value.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the offset is applied to the numerical value of the resolved value and the offset option is also retained in the resolved options, doesn't that mean that it may get double-applied?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the trouble with using :integer (or :number) for both formatting and selection.

So selection you need both the offset, and the original value.
Because you test the exact values against the original value, and the keywords against the value - offset.

But for formatting the value - offset is enough, and you can drop the offset from the resolved options.

Which means that if one does

.input {$count :integer offset=2}
.match $count
...

then the resolved value for $count should have the offset.

If we separated the :input / :number from :plural this would have an easy answer: :plural keeps the resolved value and resolved options "as is", and :integer resolves the value to the value - offset and remove the offset from the resolved values.

I am not pushing to revert that decision.
We voted and all.

But I am open to suggestions, because I am not sure what to do here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea: leave the resolved value as is, and leave the offset in the resolved options.
Then the selection can see both.
And the "formatted value" (which we don't have a concept of, although it would be useful) can do the diff.

Trouble is that without the concept of "formatted value" we can't fully describe how the formatting works.

We have a section named "Numeric Value Selection and Formatting", but we don't seem to describe the formatting part at all.

spec/registry.md Outdated
Comment on lines 159 to 160
- `offset` (optional)
- ([digit size option](#digit-size-options), default: `0`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why a negative offset is not allowed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't imagine a use case for it.
ICU works. But also works with fractional values. And negative fractional values. And with fractional arguments (see my other comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose is to list the first n members of a list explicitly, then switch on the remaining number. Negative doesn't have any usage.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ICU compatibility sounds like a good argument to me.

Copy link
Member

@aphillips aphillips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review

spec/registry.md Outdated Show resolved Hide resolved
@@ -449,14 +457,19 @@ Number selection has three modes:
- `ordinal` selection matches the operand to explicit numeric keys exactly
followed by an ordinal rule category if there is no explicit match

When the selection mode is `plural` implementations can optionaly support
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wouldn't ordinal work the same way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wouldn't ordinal work the same way?

I think we discussed it in the last meeting and nobody was able to come up with a use case for ordinals.

But ICU supports it.
So I am open to add it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list = { Audrey, Bill, Carlos, David, Edna, Freda}

.local $first = {$list :get item=1}
.local $second = {$list :get item=2}
.local $user = {$list :get item=last}
.local $length = {$list :length}
.local $position = {$length :integer select=ordinal offset=2} 
.match $position
one {{After {$first} and {$second}, {$user} is {$position}st in line}}
two {{After {$first} and {$second}, {$user} is {$position}nd in line}}
... etc...

... but it's a pretty contrived use case...

@mihnita
Copy link
Collaborator Author

mihnita commented Nov 7, 2024

If the option is available on :number, it should also be on :integer.

OTOH, if it only supports whole number values, it might make sense to only allow on :integer; I'm not aware of any use case for it having been presented for non-integer values.

I also submitted #927 as the text around resolved value should be clarified.

Good catch, thank you.

I added the option to :integer

About removing it from :number, I am split.

I can't imagine a use case for non-int with offset.

BUT I have two reasons to keep it:

One is that ICU supports it.

You can pass a non-int argument and get "Alice, Bob, and 3.142 other guests".
You can also pass a non-int offset, and works.
And you can pass a negative offset (integer or not) and works.

I cannot imagine a use case for any of these...

The second reason I kept it was that I suspect that some devs would not be too careful about :number and :integer.
So functionally they would only pass integral values, but do the formatting and selection with :number.


But I am fine to say: no, we don't support any of this.
We only support 0 or positive integers for offset, and only only support offset on :integer.

Adding them later would be backward compatible.

@mihnita
Copy link
Collaborator Author

mihnita commented Nov 7, 2024

Summarizing the questions:

  • should we support fractional and / or negative offsets?
  • should we support offset on :number?
  • should we support offset in ordinal mode?
  • what to do with offset in the resolved options?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LDML46.1 MF2.0 Draft Candidate registry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants