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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved pluralization support for gettext #135

Merged
merged 1 commit into from
Mar 21, 2016

Conversation

keichan34
Copy link
Contributor

Summary of changes

As discussed in #125

There are quite a few changes -- this is more of a "see if this is the direction you want to go in" PR rather than a "please merge this immediately" PR. There are a few more areas where I'd like to refine in the near future -- adding a few more tests, for example. Let me know what you think. 馃槂

Checklist

  • New functions have typespecs, changed functions were updated
  • Same for documentation, including moduledocs
  • Tests were added or updated to cover changes
  • Commits were squashed into a single coherent commit

I've renamed the Russian locale name from "ru_RU" to "ru", since that's what elixir-lang/gettext uses in its pluralization definition rules (see https://github.com/elixir-lang/gettext/blob/8808628da33e473d2a5af1d693bdf3373a26bb22/lib/gettext/plural.ex#L240 for more details.)

@bitwalker
Copy link
Owner

@keichan34 Hey, sorry for the delay, it was a busy week! I think the change to use lngettext is a good one, after a more thorough reading of the gettext docs, it definitely seems like that's the proper way to handle pluralization. It also makes sense to change the locale name to match what's in the pluralizer. What are the next steps before you'd like this to be merged?

@keichan34
Copy link
Contributor Author

@bitwalker I just want to get a few more tests in there and go over it again to see if I forgot to remove some code that wasn't being used anymore, general polishes, etc. I should be able to get that done tonight or tomorrow.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 70.611% when pulling 4e5eeac on keichan34:gettext-pluralization into 76b3e11 on bitwalker:master.

* Renamed "ru_RU" to "ru", since that's what elixir-lang/gettext uses in
  its pluralization definition rules (see
  https://github.com/elixir-lang/gettext/blob/8808628da33e473d2a5af1d693bdf3373a26bb22/lib/gettext/plural.ex#L240
  for more details.)
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 70.698% when pulling 93fadb8 on keichan34:gettext-pluralization into 2115930 on bitwalker:master.

@keichan34
Copy link
Contributor Author

@bitwalker Just one little change, I removed I10n.Translator.get_symbols/1 because it wasn't being used anywhere else. Do you have plans for this? Otherwise, unless you have any things you want me to fix, I think it's ready for merge.

@bitwalker
Copy link
Owner

@keichan34 Looks good to me, I'm thumbs up on the changes. I'm going to merge this (and I have to fix a typo in the folder structure, just realized I typed "i10n" instead of "l10n" (I vs L).

bitwalker added a commit that referenced this pull request Mar 21, 2016
Improved pluralization support for gettext
@bitwalker bitwalker merged commit 0aaa344 into bitwalker:master Mar 21, 2016
@keichan34 keichan34 deleted the gettext-pluralization branch March 22, 2016 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants