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

Replace Spree.t with plain I18n.t #2309

Merged
merged 17 commits into from
Oct 26, 2017

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Oct 19, 2017

The Spree.t method provides debatable benefit over the standard I18n.t method and t helper.

  • Spree.t(:foo) is one character shorter than t('spree.foo')
  • Spree.t('foo.bar') is the same length as t('spree.foo.bar')
  • Spree.t doesn't support the t(".relative") shorthand (so we currently use both t and Spree.t)
  • Spree.t generates <span class="translation_missing" on errors (like the t helper), which is surprising because it looks more like I18n.t (which doesn't emit html)
  • Use of Spree.t prevents us from using tools like i18n-tasks

This PR replaces Spree.t with I18n.t throughout the project.

@jhawthorn jhawthorn added the WIP label Oct 19, 2017
@cbrunsdon
Copy link
Contributor

👍

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Yeah, good work.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Reasonable change. Most of the commits can be stashed into one I guess.

I see the benefit of this and am for doing this, but this forces us to take a extra look while reviewing PRs that introduces translation keys. I am fine with that, though

@jhawthorn jhawthorn force-pushed the replace_spree_t_with_i18n branch 2 times, most recently from feed9dc to ea28e8d Compare October 20, 2017 22:47
@solidusio solidusio deleted a comment from houndci-bot Oct 20, 2017
@solidusio solidusio deleted a comment from houndci-bot Oct 20, 2017
@solidusio solidusio deleted a comment from houndci-bot Oct 20, 2017
@solidusio solidusio deleted a comment from houndci-bot Oct 20, 2017
@jhawthorn jhawthorn changed the title [RFC] Replace Spree.t with plain I18n.t Replace Spree.t with plain I18n.t Oct 21, 2017
@jhawthorn jhawthorn force-pushed the replace_spree_t_with_i18n branch from d8671f2 to 30b4eba Compare October 26, 2017 18:51
@jhawthorn jhawthorn merged commit 63b8a55 into solidusio:master Oct 26, 2017
@BenMorganIO BenMorganIO mentioned this pull request Jun 14, 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.

4 participants