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

default_fallbacks configuration option doesn't work (AR, Postgres) #81

Closed
smaximov opened this issue Aug 31, 2017 · 9 comments
Closed

Comments

@smaximov
Copy link

smaximov commented Aug 31, 2017

Setting default_fallbacks configuration option doesn't have any (visible) effect. Consider the following snippet:

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

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

  gem 'activerecord', '5.1.0'
  gem 'mobility', '0.2.2'
  gem 'pg', '0.18.4'
  gem 'rspec', '3.6.0', require: 'rspec/autorun'
end

DB = ENV.fetch('MOBILITY_TEST_DB', 'mobility-test').freeze

ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: DB)
ActiveRecord::Base.logger = Logger.new($stdout)

I18n.default_locale = :en
I18n.available_locales = %i[en ru]

Mobility.configure do |config|
  config.default_backend = :jsonb
  config.accessor_method = :translates
  config.default_fallbacks = { ru: :en, en: :ru }
end

ActiveRecord::Schema.define do
  create_table :articles, force: true do |t|
    t.jsonb :title, null: false, default: {}
  end
end

class Article < ActiveRecord::Base
  extend Mobility

  translates :title
end

RSpec.describe 'Fallbacks' do
  around do |example|
    I18n.with_locale(:en) { Article.create!(title: 'Title') }
    I18n.with_locale(:ru) { example.run }
  end

  subject(:article) { Article.last }

  it { expect(article.title).to eq('Title') }
end

I expected that setting default_fallbacks will make all translated attributes to fallback to existing locales by default. Moreover, when I try to use both default_fallbacks and the fallbacks option to translates, I get "default_fallbacks: undefined method call for {:ru=>:en, :en=>:ru}:Hash (NoMethodError)":

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

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

  gem 'activerecord', '5.1.0'
  gem 'mobility', '0.2.2'
  gem 'pg', '0.18.4'
end

DB = ENV.fetch('MOBILITY_TEST_DB', 'mobility-test').freeze

ActiveRecord::Base.establish_connection(adapter: 'postgresql', database: DB)
ActiveRecord::Base.logger = Logger.new($stdout)

I18n.default_locale = :en
I18n.available_locales = %i[en ru]

Mobility.configure do |config|
  config.default_backend = :jsonb
  config.accessor_method = :translates
  config.default_fallbacks = { ru: :en, en: :ru }
end

ActiveRecord::Schema.define do
  create_table :articles, force: true do |t|
    t.jsonb :title, null: false, default: {}
  end
end

class Article < ActiveRecord::Base
  extend Mobility

  translates :title, fallbacks: { ru: :en, en: :ru }
end

So I assume that either default_fallbacks is broken or the documentation is unclear on how to properly use it.

Context

  • ruby 2.4.1
  • active_record 5.1.0
  • mobility 0.2.2
@smaximov
Copy link
Author

smaximov commented Aug 31, 2017

Looks like I've got it wrong. default_fallback= takes a #call-able which is then used to create a new instance of I18n::Locale::Fallbacks hash (I find the name rather misleading), setting default fallbacks is done via default_options:

diff --git a/app.rb b/app.rb
index 4ac479e..c569251 100644
--- a/app.rb
+++ b/app.rb
@@ -25,7 +25,7 @@ I18n.available_locales = %i[en ru]
 Mobility.configure do |config|
   config.default_backend = :jsonb
   config.accessor_method = :translates
-  config.default_fallbacks = { ru: :en, en: :ru }
+  config.default_options = { fallbacks: { ru: :en, en: :ru } }
 end

 ActiveRecord::Schema.define do

This change makes the test pass.

@shioyama
Copy link
Owner

I find the name rather misleading

You're right, now that I look at it again it is rather misleading. I'll think about changing the naming for 0.3.

@shioyama
Copy link
Owner

p.s. if you have a suggestion for naming that would be more intuitive, please let me know 😄

@shioyama
Copy link
Owner

I think one thing I can do is to emit a warning if you try to set default_fallbacks to anything but a Proc-like object.

@smaximov
Copy link
Author

if you have a suggestion for naming that would be more intuitive, please let me know

How about fallbacks_factory= or default_fallbacks_proc=?

PS. I think the real issue is not the naming, it's that the docs are a bit lacking and the interface is not consistent.

First, you have #default_fallbacks (which is like an attribute getter, but in fact can also take an argument) documented, but not #default_fallbacks= (the setter), which is more important since the user will likely ever use the latter than the former.

Second, the setter and the getter has "incompatible" types in the sense that one would expect the setter to take I18n::Locale::Fallbacks (or a Hash, at least), not a #call-able if the getter returns I18n::Locale::Fallbacks. And the user has no way to know it without digging in the source code.

PPS. Thanks for the gem. I am looking for an alternative to migrate from Globalize, and I like it so far.

@smaximov
Copy link
Author

smaximov commented Aug 31, 2017

I think one thing I can do is to emit a warning if you try to set default_fallbacks to anything but a Proc-like object.

That can help, but I would make it an exception instead of a warning.

@shioyama
Copy link
Owner

You're right about the interface for the fallbacks instance. I need to think a bit about restructuring the Configuration class altogether, and how plugins use it.

The fallbacks instance setter/getter stuff was not really given a great deal of thought when I first put it in there. The default_fallbacks and default_accessor_locales stuff feels like it should not be in the global configuration at all, maybe instead encapsulated within the plugin module itself, or at least set from the plugin on the global configuration.

TBH I doubt anybody is actually using anything other than I18n::Locale::Fallbacks anyway...

@pwim
Copy link
Collaborator

pwim commented Sep 7, 2017

I also was confused by this, as I also set default_fallbacks. I think part of the confusion stems from the prominence in the documentation, as it is mentioned in the Fallbacks section, whereas setting fallbacks globally via default_options is not.

Seeing as it seems like most people won't be changing default_fallbacks, and if they do, they should know what they're doing, I'd consider removing mention of it from the readme.

More generally, I think an example of all the config options, and how you use them would be helpful.

@shioyama
Copy link
Owner

Making some improvements to the documentation for default fallbacks in #148.

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

No branches or pull requests

3 participants