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

Use carmen to translate available_countries helper #2537

Merged
merged 3 commits into from
Feb 8, 2018

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Jan 27, 2018

WIP: will revisit and test properly next week

In solidusio/solidus_i18n#106 we are looking to remove a feature from solidus_i18n which provides translations for each country based on the translations in the i18n_data gem.

We don't want to support this odd magic feature as part of i18n, and we'd like to remove the dependency on i18n_data, but at the same time we still want countries to be localized properly.

Ideally this would also be faster than the existing implementation as available_countries is a little bit slow.

This PR replaces the the existing behaviour of using I18n.t("spree.country_names.#{iso_code}") with using Carmen's data to perform translations.

This is the first time we are actually requiring and using carmen outside of tests and seeds, so we should consider the effect this has on startup time and memory usage.

TODO

  • Continue to support overrides from I18n spree.country_names.*
  • Change Carmen.i18n_backend with each request
  • Benchmark
  • Consider startup time and memory usage

@jhawthorn jhawthorn added the WIP label Jan 27, 2018
@jhawthorn
Copy link
Contributor Author

jhawthorn commented Jan 29, 2018

After

 available_countries    223.391  (± 4.5%) i/s -      1.116k in   5.006031s

Before

 available_countries     80.574  (± 6.2%) i/s -    406.000  in   5.057310s
> Benchmark.ms { require 'carmen' }
=> 6.585271989228204

@jhawthorn jhawthorn changed the title [WIP] Use carmen to translate countries Use carmen to translate countries Jan 31, 2018
@jhawthorn jhawthorn changed the title Use carmen to translate countries Use carmen to translate available_countries helper Jan 31, 2018
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks John!

@kennyadsl
Copy link
Member

I'm preparing a PR on solidus_i18n to remove that feature.

kennyadsl added a commit to solidusio/solidus_i18n that referenced this pull request Feb 7, 2018
This file was extending I18n.backend to lookup to country names
using the translation files. This will not be needed anymore
since this lookup is done via Carmen gem after solidusio/solidus#2537
@jhawthorn jhawthorn merged commit b84e28b into solidusio:master Feb 8, 2018
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