PluralRule for DualFromZeroToTwo: Does not cover value of 2 - Closes #369 #370
Conversation
PluralRule for DualFromZeroToTwo: Does not cover value of 2 The `Dictionary<string, PluralRuleDelegate> IsoLangToDelegate` is backed by a default dictionary. It can be restored with `PluralRules.RestoreDefault()` if one of the delegates was changed. Both has global effect. Extract class `CustomPluralRuleProvider` to its own file `PluralRuleDelegate DualFromZeroToTwo`: * with 3 words, the index is for counts of 0, > 0 and < 2, more than 2 * with 4 words, the index is for counts of 0, > 0 and < 2, >= 2 and < 3, more than 3 Add unit tests
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #370 +/- ##
===================================
Coverage 96% 96%
===================================
Files 91 92 +1
Lines 3227 3233 +6
===================================
+ Hits 3111 3117 +6
Misses 116 116 ☔ View full report in Codecov by Sentry. |
| <=0 => 0, | ||
| > 0 and < 2 => 1, | ||
| >= 2 and < 3 => 2, | ||
| _ => 3 | ||
| }; |
There was a problem hiding this comment.
@karljj1 While going through the code once again:
Since the very beginning, PluralRules are dealing with decimal, while comparisons are integer based. As soon as decimal is not an integer, the index for "other" is returned. See also the Between(...) method. Can you figure out, what was the intention behind?
If this is the concept, these range comparisons break the concept, don't they?
There was a problem hiding this comment.
Im not sure I understand. The Between method uses decimals. Maybe Im misunderstanding you?
There was a problem hiding this comment.
Yes, Between uses decimal, but checks that there is no faction (like 1.0000001). That's what I mean: The comparison there (and in other methods as well) expects that there's no fraction. The range comparison includes fractions and is inconsistent with the other comparisons in this respect, isn't it?
Frankly I don't understand why this kind of implementation was chosen, and I wonder whether there's something I'm just missing.
There was a problem hiding this comment.
Which range comparison are you referring to that uses fractions?
There was a problem hiding this comment.
I'm not the author of PluralRules, and I think there is an inconsistency. I can't imagine the reason for it.
See these examples:
// Never include fractional numbers to return true
Assert.That(PluralRules.BetweenWithoutFraction(1.2M, 1M, 2M), Is.False);
Assert.That(PluralRules.BetweenWithoutFraction(1.0M, 1M, 2M), Is.True); // whole number
// Never include fractional numbers
Assert.That(PluralRules.Breton(1M, default), Is.EqualTo(1));
Assert.That(PluralRules.Breton(1.1M, default), Is.EqualTo(5)); // default, not a whole number
// Always include fractional numbers with DualFromZeroToTwo
// Why only here?
Assert.That(PluralRules.GetWordsCount4Value(1M), Is.EqualTo(1));
Assert.That(PluralRules.GetWordsCount4Value(1.1M), Is.EqualTo(1)); // same result for number with fractionWhat I'm trying to say: Range comparisons for a value are only used with PluralRules.DualFromZeroToTwo. This is not consistent or must have a good reason. So what is it?
My guess is, that range comparisons would be the right way for decimal (!) values. Why should we use decimal, if we compare for equality with whole numbers? Then we could use long as well.
There was a problem hiding this comment.
Yeah, I don't know why it's this way. There is some mention in the CLDR that the many option can also be used for fractions https://cldr.unicode.org/index/cldr-spec/plural-rules
Fractions
In some languages, fractions require a separate category. For example, Russian 'other' in the example above. In some languages, they all in a single category with some integers, and in some languages they are in multiple categories. In any case, they also need to be examined to make sure that there are sufficial minimal pairs.
Fractions are often a bit tricky to determine: languages have very different behavior for them. In some languages the fraction is ignored (when selecting the category), in some languages the final digits of the fraction are important, in some languages a number changes category just if there are visible trailing zeros. Make sure to try out a range of fractions to make sure how the numbers behave: values like 1 vs 1.0 may behave differently, as may numbers like 1.1 vs 1.2 vs 1.21, and so on.
Maybe the original used an older version of the CLDR?
Fractional support in plurals is new in CLDR 24. Because the fractions didn't work before, the changes in categories from 23 to 24 should not cause an issue for implementations.
There was a problem hiding this comment.
Thanks a lot, Karl, with your hint to Fraction support the scales fell from my eyes! ½, 1 ½ sure!
I'll revert to v3.3.0 code, but include 2 as the minimum plural value. Then this is just a bug fix with no breaking changes.
Revert to v3.3.0 code, but include 2 as the minimum plural value
|
axunonb
changed the title
PluralRule for DualFromZeroToTwo: Does not cover value of 2
The
Dictionary<string, PluralRuleDelegate> IsoLangToDelegateis backed by a default dictionary. It can be restored withPluralRules.RestoreDefault()if one of the delegates was changed. Both has global effect.Extract class
CustomPluralRuleProviderto its own filePluralRuleDelegate DualFromZeroToTwo:Add unit tests