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

allow disabling fallback explicitly when calling translate #354

Merged
merged 1 commit into from
Feb 1, 2017

Conversation

ignatiusreza
Copy link
Contributor

e.g. by doing I18n.translate(:key, :fallback => false)

note that currently, we sort of able to do this by doing I18n.translate(:key, :fallback => true), which are quite a confusing behavior, introduced -- I assumed -- unintentionally because we're modifying options.. so this PR does have backward incompatibility changes, even though it's one that was not officially supported before?

another note is, in the wiki for fallbacks, we have something like:

You can skip the fallback and return the default by passing in an empty fallback array

I18n.translate(:foo, :locale => :de, :default => 'Default Foo', :fallbacks => []) # => 'Default Foo'

which does not seems to works.. if what the wiki is saying is the preferred way for supporting disabling fallback, I'm happy to change this PR to do just that, though, I must say that having both fallback and fallbacks as part of the options will be confusing..

@ignatiusreza
Copy link
Contributor Author

seems like test is broken in master too, opened a separate PR to fix it at #355 .. will rebase later..

@radar
Copy link
Collaborator

radar commented Dec 29, 2016

Does I18n.translate(:foo, :locale => :de, :default => 'Default Foo', :fallbacks => []) work already with the master version? Note that the option here is fallbacks (plural) and not fallback.

e.g

I18n.translate(:key, :fallback => false)
@ignatiusreza
Copy link
Contributor Author

nope, as far as I can tell, fallbacks option doesn't work in master, despite what the wiki are saying..

@radar
Copy link
Collaborator

radar commented Jan 23, 2017

@ignatiusreza Would you please be able to provide me with a small reproduction repo that I could use to investigate this issue? I think you'd be better at coming up with one than me because it sounds like you understand what's happening here.

Please provide steps to reproduce the issue and expected/actual outcomes as well.

From that point I should have enough information to investigate.

Thanks!

@ignatiusreza
Copy link
Contributor Author

Sure @radar ! Would something like the following be okay?

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  # use this to see test #2 pass, while #1 & #3 fails
  gem "i18n"

  # use this to see test #3 pass, while #1 & #2 fails
  # gem "i18n", github: 'ignatiusreza/i18n', branch: 'disable_fallback'

  gem "minitest"
  gem "test_declarative"
end

require "i18n"
require "minitest/autorun"

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

class I18nBackendFallbacksTranslateTest < Minitest::Test
  # ehem.. I don't actually suck, but it's easier to follow if the test are sorted.. :p
  i_suck_and_my_tests_are_order_dependent!

  class Backend < I18n::Backend::Simple
    include I18n::Backend::Fallbacks
  end

  def setup
    super

    I18n.enforce_available_locales = false

    I18n.backend = Backend.new
    I18n.backend.store_translations(:en, :foo => 'Foo in :en', :bar => 'Bar in :en')
    I18n.backend.store_translations(:de, :bar => 'Bar in :de')
  end

  def teardown
    I18n.locale = nil
    I18n.default_locale = nil
    I18n.load_path = nil
    I18n.available_locales = nil
    I18n.backend = nil
    I18n.default_separator = nil
    I18n.enforce_available_locales = true

    super
  end

  test "1. disable fallback using :fallbacks => []" do
    # syntax taken from wiki, but it doesn't works
    # ref: https://github.com/svenfuchs/i18n/wiki/Fallbacks#providing-a-default
    assert_equal 'Default Foo', I18n.t(:foo, :locale => :de, :default => 'Default Foo', :fallbacks => [])
  end

  test "2. disable fallback using :fallback => true" do
    # side effect of modifying options[:fallback]
    # ref: https://github.com/svenfuchs/i18n/commit/957145ef85875b913a93502111368b4fc2c67fd5
    assert_equal 'Default Foo', I18n.t(:foo, :locale => :de, :default => 'Default Foo', :fallback => true)
  end

  test "3. disable fallback using :fallback => false" do
    # suggested approach
    # ref: https://github.com/svenfuchs/i18n/pull/354
    assert_equal 'Default Foo', I18n.t(:foo, :locale => :de, :default => 'Default Foo', :fallback => false)
  end
end

@radar
Copy link
Collaborator

radar commented Jan 31, 2017

Thank you @ignatiusreza. This is excellent.

I don't see anywhere in the code that uses the passed-in fallbacks option, so I am guessing the wiki is out-of-date with that one.

I have taken a look at this issue this morning and I can't figure out how to fix it without breaking other things. Maybe someone smarter than me can figure it out?

@ignatiusreza
Copy link
Contributor Author

I'm sorry, but could you help clarify in what you meant by without breaking other things.? since test passes even with my changes.. or are there any other scenario which are untested that could be broken?

if you're worried about making fallback: true stop working, then maybe we can have a deprecation cycle which provide a deprecation warning when translate is called with explicit truthy value for options[:fallback]? though, since we're retiring an undocumented behavior, I'm not sure if it's worth it to have a deprecation cycle..

@radar
Copy link
Collaborator

radar commented Feb 1, 2017

Sorry, I had forgotten this was a PR and I was writing those replies on my phone. Now that I'm on a computer, I can see better :) Sorry for that confusion.


The code looks great and the tests are all happy, and so am I :)

@radar radar merged commit c913b0a into ruby-i18n:master Feb 1, 2017
@ignatiusreza ignatiusreza deleted the disable_fallback branch February 2, 2017 00:17
@ignatiusreza
Copy link
Contributor Author

not a problem.. thanks for the merge 👍

@jbourassa
Copy link

We were using fallback: [] as a way of having no fallbacks (no s at fallback). That worked, although incorrectly documented in the Wiki. I was wondering why it stopped working so I git bisect'ed my way to here. I think it's a good change, it is almost 1 year old and I don't see any complaints about it, so I'll just update the wiki and our codebase – I don't think it's worth resurrecting the old behaviour at this point.

@radar
Copy link
Collaborator

radar commented Jan 22, 2018

@jbourassa Thanks for the comment and for updating the wiki :)

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.

3 participants