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

Add missing-plural-keys #309

Merged
merged 5 commits into from
Oct 25, 2018
Merged

Conversation

Gargron
Copy link
Contributor

@Gargron Gargron commented Oct 24, 2018

Fix #308

@Gargron Gargron force-pushed the feature-missing-plural-keys branch 2 times, most recently from 0fd4178 to 1bd2ddc Compare October 24, 2018 04:51
@Gargron
Copy link
Contributor Author

Gargron commented Oct 24, 2018

@glebm So here's the deal. If you require i18n/tasks from a Rails app that has rails-i18n as a dependency, then the method works and returns all nodes that have missing pluralizations.

But from within the standalone executable, it does not work. I cannot figure out how to include pluralization data from rails-i18n into the gem itself. Without pluralization data, nothing can be done.

@Gargron Gargron force-pushed the feature-missing-plural-keys branch from 1bd2ddc to 3d7a182 Compare October 24, 2018 04:55
@glebm
Copy link
Owner

glebm commented Oct 24, 2018

I cannot figure out how to include pluralization data from rails-i18n into the gem itself

You'd need to add dependency on rails-i18n to this gem.

I also think this should be part of the missing task. Tasks like add/translate-missing should also have special behaviour for plural keys. What do you think?

You'd need to add a new missing "type" :plural here and go from there:

# @param types [:used, :diff] all if `nil`.
# @return [Siblings]
def missing_keys(locales: nil, types: nil, base_locale: nil)

@Gargron Gargron force-pushed the feature-missing-plural-keys branch from 3d7a182 to f9aec5c Compare October 24, 2018 19:17
@Gargron
Copy link
Contributor Author

Gargron commented Oct 24, 2018

Okay, I was able to add rails-i18n as a dependency. I thought it would be more trouble because all that does by itself is define a Railtie, which wouldn't work without a Rails environment, but I was able to use on of the methods to just load all pluralization rules. rails-i18n itself defines i18n-tasks as a development dependency, I hope there's no ill effects from that circularity...

Now, I ensured that missing-plural-keys works as a command. I'll try to move it to the missing keys in a different commit...

@Gargron Gargron force-pushed the feature-missing-plural-keys branch 2 times, most recently from 8812d66 to 60e823e Compare October 24, 2018 20:42
@Gargron
Copy link
Contributor Author

Gargron commented Oct 24, 2018

There are some rubocop offences I can't really fix

@Gargron Gargron force-pushed the feature-missing-plural-keys branch from 60e823e to 7a57fbe Compare October 24, 2018 20:46
@glebm
Copy link
Owner

glebm commented Oct 25, 2018

Feel free to selectively disable Rubocop checks where it makes sense (see https://github.com/rubocop-hq/rubocop/blob/master/manual/configuration.md#disabling-cops-within-source-code)

rails-i18n itself defines i18n-tasks as a development dependency, I hope there's no ill effects from that circularity

Curious. I guess we'll soon find out. 😄

@Gargron Gargron force-pushed the feature-missing-plural-keys branch from 7a57fbe to c421e75 Compare October 25, 2018 08:43
@Gargron
Copy link
Contributor Author

Gargron commented Oct 25, 2018

Done!

It's now part of missing. I have also removed some dead code that was not called from anywhere, such as glyph/summary definitions for the missing types, and the glyph renderer in the terminal report.

Also, rails-i18n does not call i18n-tasks code, only the stand-alone executable, so I don't think it will be a problem.

I do not have the bandwidth to implement an add-missing behaviour here. My guess is just that if the translations for all plural keys are identical, the translation should be turned from a tree to a single leaf. It would be convenient but I don't really need it right now.

@glebm glebm merged commit efcc22c into glebm:master Oct 25, 2018
@glebm
Copy link
Owner

glebm commented Oct 25, 2018

Thank you! This and interpolations are great additions!

@Gargron Gargron deleted the feature-missing-plural-keys branch October 25, 2018 19:30
glebm added a commit that referenced this pull request Oct 26, 2018
glebm added a commit that referenced this pull request Oct 26, 2018
glebm added a commit that referenced this pull request Oct 26, 2018
glebm added a commit that referenced this pull request Oct 26, 2018
glebm added a commit that referenced this pull request Oct 26, 2018
@SirRawlins
Copy link

@Gargron - Is there any way to ignore missing plural keys? I've tried adding the missing keys to ignore_missing missing array in my configuration, however, they still get flagged as missing.

bartimaeus added a commit to bartimaeus/i18n-tasks that referenced this pull request Feb 8, 2019
* upstream/master: (177 commits)
  ✏️ Typo
  📝 Add example of usage for "Key pattern syntax"
  Silence warnings for common leaf -> tree expansion scenario.
  Update README and post installation instructions message (glebm#320)
  Bump to v0.9.28
  Do not consider a plural node if it only has one child with no count (glebm#312)
  Include file name in CommandError raised during unsuccessful file load (glebm#313)
  Use separate trees per locale in missing_plural_forest (glebm#311)
  Missing plural keys: Extract plural_nodes method
  Missing plural keys: minor optimization
  Missing plural keys: do not modify data nodes
  Check for missing plural keys (glebm#309)
  Fix interpolations with repeated vars
  readme [ci skip]
  changelog
  s/inconsistent_interpolation/inconsistent_interpolations
  Bump to v0.9.26
  s/Commands::Inconsistent/Commands::Interpolations/
  DeeplTranslator: Fix CommandError
  Follow-up to glebm#304
  ...
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.

3 participants