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

Unify/simplify the behavior of per-locale and non-per-locale files by moving per-locale generation to Segment class #320

Merged
merged 4 commits into from
Apr 1, 2015

Conversation

johnnyshields
Copy link

This PR unifies/simplifies the behavior of per-locale and non-per-locale files.

Specifically, splitting files per-locale generation is done in the Segment class .save! method, which is the final step before writing the file. (Previously, "per-locale" and "non-per-locale" files followed very different logic paths which diverged early in the processing chain.)

One positive effect of this change is that the "fallbacks" feature now works for both per-locale and non-per-locale files.

Important: There are two breaking changes in this PR, hence I recommend a minor version bump.

  1. Per-locale files should now have only/exclude scopes which begin with '*', which is the same as non-per-locale files.
  2. Since this PR fixes the issue that fallbacks were ignored for non-per-locale files, users may see some behavior change. for this. This is unavoidable as it's essentially a bug that this wasn't done before.

Lastly, please note that I had to add fallbacks: false to many of the test fixtures to ensure behavior consistency.

… moving per-locale generation to Segment class
@@ -263,6 +283,8 @@
end

context "I18n.available_locales" do
# before { allow(I18n::JS).to receive(:fallbacks).and_return(false) }

Choose a reason for hiding this comment

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

Commented code o_0

@johnnyshields
Copy link
Author

All comments have been addressed.

@PikachuEXE PikachuEXE self-assigned this Mar 30, 2015
@PikachuEXE
Copy link

For your breaking changes:

  • For (2), it's fine, it was a bug
  • For (1), It's not clear to me for what you are talking about.
    Please clarify it (or write README and add CHANGELOG entry directly :P

@johnnyshields
Copy link
Author

To explain (1):

Old format. Note that the %{locale} one doesn't begin with '*'

- file: '/my_dir/not_per_locale.js'
  only:
    - '*.foo.*'
    - '*.bar.*'

- file: '/my_dir/my_%{locale}.js'
  only:
    - 'foo.*'
    - 'bar.*'

New format:

- file: '/my_dir/not_per_locale.js'   # same
  only:
    - '*.foo.*'
    - '*.bar.*'

- file: '/my_dir/my_%{locale}.js'   # now has *
  only:
    - '*.foo.*'
    - '*.bar.*'

@PikachuEXE
Copy link

OK, I think that's more consistent and sensible.
Please update README and CHANGELOG so I can merge this.
Thanks :D

@johnnyshields
Copy link
Author

Updated README and CHANGELOG

@PikachuEXE
Copy link

Common mistake: you have added the changelog entry for a released version o_0

@johnnyshields
Copy link
Author

Fixed

PikachuEXE added a commit that referenced this pull request Apr 1, 2015
Unify/simplify the behavior of per-locale and non-per-locale files by moving per-locale generation to Segment class
@PikachuEXE PikachuEXE merged commit cdeed14 into fnando:master Apr 1, 2015
@michieldewit
Copy link

For the except option the *.something.* format is not supported as is for the only option. It only supports key names. So when errors is specified for the except option, all of the following segments will be excluded: errors, app.errors, app.view.errors. It seems to have always been like this. Please adjust comments in CHANGELOG/README or change the functionality (the latter I would prefer).

@PikachuEXE
Copy link

@michieldewit Please open another issue :)

@michieldewit
Copy link

I created a pull request for this: #325

@PikachuEXE
Copy link

Will review it next week <3

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