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

Fix: PluralRules for Czech locale #325

Merged
merged 3 commits into from
Dec 16, 2022
Merged

Fix: PluralRules for Czech locale #325

merged 3 commits into from
Dec 16, 2022

Conversation

alexheb
Copy link
Contributor

@alexheb alexheb commented Dec 5, 2022

I added new Czech PluralRuleDelegate according to Czech grammar. It would break the usage of an old rule, however I don't believe it could have been used in real word. The test should be:

public void SmartFormatPluralTest()
{
    var c = new CustomPluralRuleProvider(Czech);
    string msg = "{0:plural:Nemáte zprávu|Máte {} zprávu|Přišly Vám {} zprávy|Přišlo Vám {} zpráv}!";
    Smart.Format(c, msg, 0).Should().Be("Nemáte zprávu!");
    Smart.Format(c, msg, 1).Should().Be("Máte 1 zprávu!");
    Smart.Format(c, msg, 2).Should().Be("Přišly Vám 2 zprávy!");
    Smart.Format(c, msg, 4).Should().Be("Přišly Vám 4 zprávy!");
    Smart.Format(c, msg, 5).Should().Be("Přišlo Vám 5 zpráv!");
    Smart.Format(c, msg, 6).Should().Be("Přišlo Vám 6 zpráv!");
}

I added new Czech PluralRuleDelegate according to Czech grammar. It would break the usage of an old rule, however I don't believe it could have been used in real word. The test should be:

    public void SmartFormatPluralTest()
    {
        var c = new CustomPluralRuleProvider(Czech);
        string msg = "{0:plural:Nemáte zprávu|Máte {} zprávu|Přišly Vám {} zprávy|Přišlo Vám {} zpráv}!";
        Smart.Format(c, msg, 0).Should().Be("Nemáte zprávu!");
        Smart.Format(c, msg, 1).Should().Be("Máte 1 zprávu!");
        Smart.Format(c, msg, 2).Should().Be("Přišly Vám 2 zprávy!");
        Smart.Format(c, msg, 4).Should().Be("Přišly Vám 4 zprávy!");
        Smart.Format(c, msg, 5).Should().Be("Přišlo Vám 5 zpráv!");
        Smart.Format(c, msg, 6).Should().Be("Přišlo Vám 6 zpráv!");
    }
@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Base: 96% // Head: 96% // Increases project coverage by +0% 🎉

Coverage data is based on head (fcf2d0f) compared to base (62c1333).
Patch coverage: 100% of modified lines in pull request are covered.

❗ Current head fcf2d0f differs from pull request most recent head 3b0d94a. Consider uploading reports for the commit 3b0d94a to get more accurate results

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #325   +/-   ##
===================================
  Coverage    96%    96%           
===================================
  Files        91     91           
  Lines      3183   3185    +2     
===================================
+ Hits       3067   3069    +2     
  Misses      116    116           
Impacted Files Coverage Δ
src/SmartFormat/Utilities/PluralRules.cs 100% <100%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@axunonb
Copy link
Member

axunonb commented Dec 5, 2022

Good catch, Alex, that seems to be consistent with this document as far as I understand without speaking Czech. Assume you're a native speaker 😉
Would be great if you added the unit test(s) to your PR.
May I ask in which context your using Smart.Format?

@alexheb
Copy link
Contributor Author

alexheb commented Dec 6, 2022

Hello,
Thank you for the reply! You sent me a great source which forced me to think more about it. So the result would be:

private static PluralRuleDelegate CzechFix => (value, pluralWordsCount) =>
value == 0 ? 0 : //zero
value == 1 ? 1 : //one
value == -1 ? 1 : //minus one => one
Math.Abs(value).Between(2, 4) ? 2 :    //few
value % 1 == 0 ? 3 :    //many (integers)
4;  //other (fractions)

As you see I noticed that SmartFormat does not serve negative values properly (look at Math.Abs used) and I believe this could be a problem in many languages..?

Note: The other comments can be discussed here: #327

@axunonb
Copy link
Member

axunonb commented Dec 11, 2022

SmartFormat does not serve negative values

Indeed, only positive arguments are implemented.
Changing this behavior would result in a breaking change, right?

Fixed version of Czech PluralRule with support of fractions.
@alexheb
Copy link
Contributor Author

alexheb commented Dec 13, 2022

Hello, so I made final Czech Rule, ignoring negative values. Well, handling them would be breaking change, however - there could be an optional switch i.e. The test for this should be (frankly I don't know where to put it):

    public void SmartFormatPluralCzechTest()   
    {
        var c = new CustomPluralRuleProvider(Czech);
        string den = "{0:plural:Žádný den|{} den|{} dny|{} dní|{} dne}!";
        Assert.Equal("Žádný den!", Smart.Format(c, den, 0));
        Assert.Equal("1 den!", Smart.Format(c, den, 1));
        Assert.Equal("2 dny!", Smart.Format(c, den, 2));
        Assert.Equal("4 dny!", Smart.Format(c, den, 4));
        Assert.Equal("5 dní!", Smart.Format(c, den, 5));
        Assert.Equal("0,5 dne!", Smart.Format(c, den, 0.5));
    }

@axunonb
Copy link
Member

axunonb commented Dec 16, 2022

ignoring negative values

I went through the unit tests, and there are tests using negative values.
It can be argued whether negative values are really useful, but currently it is possible. Limiting to positive values might be undesirable for some (unknown) cases.

@axunonb axunonb changed the title Update PluralRes to fix Czech plural rules Fix: PluralRules for Czech locale Dec 16, 2022
axunonb
axunonb previously approved these changes Dec 16, 2022
@axunonb axunonb merged commit 4c5a072 into axuno:main Dec 16, 2022
axunonb added a commit to axunonb/SmartFormat that referenced this pull request Dec 16, 2022
* Update PluralRes to fix Czech plural rules

I added new Czech PluralRuleDelegate according to Czech grammar. It would break the usage of an old rule, however I don't believe it could have been used in real word. The test should be:

    public void SmartFormatPluralTest()
    {
        var c = new CustomPluralRuleProvider(Czech);
        string msg = "{0:plural:Nemáte zprávu|Máte {} zprávu|Přišly Vám {} zprávy|Přišlo Vám {} zpráv}!";
        Smart.Format(c, msg, 0).Should().Be("Nemáte zprávu!");
        Smart.Format(c, msg, 1).Should().Be("Máte 1 zprávu!");
        Smart.Format(c, msg, 2).Should().Be("Přišly Vám 2 zprávy!");
        Smart.Format(c, msg, 4).Should().Be("Přišly Vám 4 zprávy!");
        Smart.Format(c, msg, 5).Should().Be("Přišlo Vám 5 zpráv!");
        Smart.Format(c, msg, 6).Should().Be("Přišlo Vám 6 zpráv!");
    }

* Update PluralRules.cs

Fixed version of Czech PluralRule with support of fractions.

* Add unit tests for Czech locale

Co-authored-by: axunonb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants