More from_str cleanups
#5182
More from_str cleanups
#5182
Conversation
robertbastian
requested review from
Manishearth,
nordzilla,
zbraniecki and
sffc
as code owners
|
|
||
| impl FromStr for PluralOperands { | ||
| #[cfg(feature = "datagen")] | ||
| impl core::str::FromStr for PluralOperands { |
There was a problem hiding this comment.
Discussion: I'm not sure we want impl FromStr for PluralOperands to be cfg(datagen), although I'm fine if we do since we have a way to get this same functionality via fixed_decimal.
There was a problem hiding this comment.
Users can create plural operands from all numeric types as well as fixed decimal, infallibly.
ffi/ecma402/src/pluralrules.rs
Outdated
| dbg!("ret={:?}\n---\n", &ret); | ||
| ret | ||
| let _nstr = fixed_format(n, &opts); | ||
| todo!() |
There was a problem hiding this comment.
what do you want me to do here? I'm tempted to just remove the whole crate
There was a problem hiding this comment.
I think endeavor to keep it working. It's not that much code to keep working.
In this case, just change PluralOperands::from_str to FixedDecimal::from_str.
ffi/ecma402/src/pluralrules.rs
Outdated
| dbg!("ret={:?}\n---\n", &ret); | ||
| ret | ||
| let _nstr = fixed_format(n, &opts); | ||
| todo!() |
There was a problem hiding this comment.
I think endeavor to keep it working. It's not that much code to keep working.
In this case, just change PluralOperands::from_str to FixedDecimal::from_str.
| #[test] | ||
| fn format_conversion() { | ||
| #[derive(Debug)] | ||
| struct TestCase { | ||
| n: f64, | ||
| opts: Options, | ||
| expected: PluralOperands, | ||
| } | ||
| let tests = [TestCase { | ||
| n: 1.5, | ||
| opts: Options { | ||
| in_type: Type::Cardinal, | ||
| minimum_integer_digits: 3, | ||
| minimum_fraction_digits: 2, | ||
| maximum_fraction_digits: 3, | ||
| minimum_significant_digits: 3, | ||
| maximum_significant_digits: 4, | ||
| }, | ||
| expected: RawPluralOperands { | ||
| i: 1, | ||
| v: 2, | ||
| w: 1, | ||
| f: 50, | ||
| t: 5, | ||
| c: 0, | ||
| } | ||
| .into(), | ||
| }]; | ||
| for test in tests { | ||
| let actual = to_icu4x_operands(test.n, test.opts.clone()); | ||
| assert_eq!(test.expected, actual, "test: {:?}", &test); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: Please restore the test for to_icu4x_operands
#4931