symbol can't be argument for :count in east slavic locales

[bitberry-dev]

Hi guys!

Symbol can't be argument for :count in east slavic locales, east slavic pluralization rule requires number because it uses modulo division, https://github.com/svenfuchs/rails-i18n/blob/master/lib/rails_i18n/common_pluralizations/east_slavic.rb

[ericsaupe]

Looks good to me!

Would you mind changing the commit message to something shorter? You can include more description on additional lines below

[jacobherrington]

@bitberries thanks for making this PR! 🎉

We have some great resources for writing good commit messages in our contributing.md file, if you're interested!

[bitberry-dev]

Changed commit message, is it okay now?

[ericsaupe]

Much better, thanks!

[kennyadsl]

@bitberries I'm not sure to get the issue with current code and the link provided does not help understanding (for me). Can you please point to some other resource or explain the issue better?

[bitberry-dev]

Just try to visit this page with russian locale and you will get exception.

[bitberry-dev]

Exception screenshot, just visit /admin/promotion_categories?locale=ru
P.S. Spree also has this bug last 2 years

[bitberry-dev]

Also I never saw that count parameter was not numerical. Everywhere it is number. It is assumed by pluralization rules that it will be a number, check it https://github.com/svenfuchs/rails-i18n/tree/master/lib/rails_i18n/common_pluralizations - everywhere comparisons only with numbers.

This code is just randomly working correctly for the English language.

[kennyadsl]

This code is just randomly working correctly for the English language.

That's what I was trying to understand, thanks. It looks like it's not supported by rails-i18n so we should avoid using it here as for your PR.

I've also found the initial work for this in Solidus: #1191

Are there other occurrences of this wrong pluralization in Solidus? It is worth to update all of them in this PR maybe, what do you think?

[bitberry-dev]

I have not found more similar errors (via grep), but if I notice something like this I will definitely make a pool request! Solidus is great project with clean code (in comparison with spree) and I will help make it better :)

[kennyadsl]

@bitberries I've just found this one:

solidus/backend/spec/features/admin/users_spec.rb

Line 40 in fac5b35

expect(page).to have_link Spree::LegacyUser.model_name.human(count: :other), href: spree.admin_users_path

This should be unsupported as well. I know that this will not break any thing since specs does not run with east slavic locale but we should not encourage bad practices in the codebase. Every line of code could be taken as example and reused again, maybe somewhere where it will be dangerous.

[bitberry-dev]

Yep, you are right. I fixed it.

[kennyadsl]

Left a question, also I think you can squash commits and change the commit message to reflect both changes, thanks!

[bitberry-dev]

Now using plural_resource_name in spec.
Squashed into one commit.

[kennyadsl]

I know I suggested to add this but thinking twice it's better to don't add implementation details to the feature specs expectations.