unicode-org / icu4x

Solving i18n for client-side and resource-constrained environments.
https://icu4x.unicode.org
Other
1.37k stars 176 forks source link

Re-consider arguments for FixedDecimal::pad_or_truncate_left #1476

Closed sffc closed 2 years ago

sffc commented 2 years ago

The function signature currently shifts the upper magnitude, like this:

let mut dec = FixedDecimal::from(42);
assert_eq!("42", dec.to_string());

dec.pad_or_truncate_left(2);
assert_eq!("0042", dec.to_string());

dec.pad_or_truncate_left(-2);
assert_eq!("42", dec.to_string());    

dec.pad_or_truncate_left(-1);
assert_eq!("2", dec.to_string());

This works. However, I think the much more common use case is to set the integer to a certain number of digits. Something like this would make more sense:

let mut dec = FixedDecimal::from(42);
assert_eq!("42", dec.to_string());

dec.pad_or_truncate_left(4);
assert_eq!("0042", dec.to_string());

dec.pad_or_truncate_left(2);
assert_eq!("42", dec.to_string());    

dec.pad_or_truncate_left(1);
assert_eq!("2", dec.to_string());

Here, the integer argument is the power of 10, rather than a delta against the power of 10.

Should we make this change?

Manishearth commented 2 years ago

Yes, I agree 100%, I found this weird when using it too. That said, I have asked the WearOS people what they prefer and haven't seen a response yet, I'll let you know what they say.

Manishearth commented 2 years ago

Opened https://github.com/unicode-org/icu4x/pull/1482

younies commented 2 years ago

I think it is a good idea. however, I feel it is also good to be the delta.