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

Disable fallbacks when using locale/fallthrough accessors #86

Merged
merged 4 commits into from
Sep 13, 2017

Conversation

shioyama
Copy link
Owner

@shioyama shioyama commented Sep 8, 2017

Fixes #85

@@ -284,7 +284,7 @@ def save
expect(article.title).to eq("Title")
expect(article.changed?).to eq(true)
expect(article.changed).to match_array(["title_ja", "title_en"])
expect(article.changes).to eq({ "title_ja" => [nil, "ばばば"], "title_en" => ["ばばば", "Title"]})
expect(article.changes).to eq({ "title_ja" => [nil, "ばばば"], "title_en" => [nil, "Title"]})
Copy link
Owner Author

Choose a reason for hiding this comment

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

As a byproduct, this change also makes dirty/fallbacks compatibility a bit less weird.

@shioyama
Copy link
Owner Author

shioyama commented Sep 8, 2017

@pwim see what you think.

@shioyama shioyama force-pushed the locale_accessors_fallbacks branch 2 times, most recently from 90ebe75 to 10466fd Compare September 8, 2017 15:01
@pwim
Copy link
Collaborator

pwim commented Sep 8, 2017

The downside to this approach is that it creates a dependency between the locale_accessors and fallbacks plugins.

locale_accessors needs to know about the fallbacks plugin because the fallbacks plugin removes the ability to ask for a fallback in a given language

Mobility.locale = :ja
word = Word.create(name: "モビリティ", meaning: "(名詞):動きやすさ、可動性")
word.name(locale: :de)
#=> "モビリティ"

Personally, I think the following behaviour would be cleaner:

Mobility.locale = :ja
word = Word.create(name: "モビリティ", meaning: "(名詞):動きやすさ、可動性")
word.name(locale: :de)
#=> nil
Mobility.locale = :de
word.name
#=> "モビリティ"

i.e., the fallbacks would only be used when the locale is implied by the global one, not explicit. If this was the case, then it would be possible to implement the accessors in a way that doesn't explicitly couple it to the fallback plugin.

@shioyama
Copy link
Owner Author

shioyama commented Sep 8, 2017

Ah, interesting. You're right that passing locale: :de should not use fallbacks either, so handling it the way you proposed would kill two birds with one stone, while also removing coupling.

@shioyama
Copy link
Owner Author

shioyama commented Sep 9, 2017

So this is the way I can envision implementing this.

Currently, whether the locale is passed into a reader or writer with locale: :de or through the global Mobility.locale, the backend and plugins sees them exactly the same. We could change this so that in the reader method, instead of simply defaulting to the global locale, we set some kind of flag which the backend or plugins can use to optionally act differently (as in the case of locale/fallthrough accessors).

Something like this:

define_method attribute do |**options|
  return super() if options[:super]
  locale = options[:locale] || Mobility.locale
  Mobility.enforce_available_locales!(locale)
  options[:locale] &&= !!locale
  mobility_backend_for(attribute).read(locale.to_sym, options)
end

So ignoring the super stuff which is unrelated: if a locale is passed in, then this is converted to a boolean and backends/plugins will see locale: true which they can interpret as meaning that the locale passed in as the first argument was explicitly set.

The changes to locale/fallthrough accessors in this PR can then be reverted since we don't need them, and instead fallbacks has to look for locale: true and in this case treat it as fallback: false. So basically rather than coupling locale accessors to fallbacks, we're instead coupling fallbacks to the core Attributes class, which is better since anyway they are already coupled.

I think using the locale key makes sense since we anyway already use it so it is "reserved" as a namespace on the accessor options hash.

@pwim
Copy link
Collaborator

pwim commented Sep 9, 2017

This approach sounds good to me. I think setting locale: true should work, and if I understand things correctly, it is only exposed to plugins anyways.

@shioyama shioyama force-pushed the locale_accessors_fallbacks branch 5 times, most recently from 13a3642 to 34a1b8b Compare September 11, 2017 12:41
@shioyama
Copy link
Owner Author

@pwim I've merged this into master, give it a try and let me know what you think. I'm debating whether to release it as 0.2.3 or combine it with other changes in the next minor version 0.3...

@shioyama shioyama deleted the locale_accessors_fallbacks branch September 14, 2017 01:02
@pwim
Copy link
Collaborator

pwim commented Sep 14, 2017

Awesome! After pointing my Gemfile to point to master, applying the workaround described in #84, and working around two differences with globalize, I've switched Doorkeeper to use mobility. It's live now, with no immediately obvious issues.

Seeing as this is making an incompatible API change, semantic versioning suggests we should release a new minor version.


By the way, I discovered two slight differences with globalize:

  1. Globalize's dirty handling marks the localized attribute as dirty (eg. title), whereas Mobility marks the locale specific attribute as dirty (eg. title_ja).
  2. When you set a translation to "" with Globalize, the localized attribute will return nil. For example Event.new(title_ja: "").title #=> nil, whereas Mobility will return the empty string.

I think eventually you'll want to make a guide for migrating from Globalize to Mobility, so I wanted to report these as they might be worth noting. Though, they might be too specific to bother with, and won't make a difference for most people.

@shioyama
Copy link
Owner Author

shioyama commented Sep 14, 2017

@pwim Thanks for investigating! And glad to hear you're using Mobility in production! That's great news.

About the two differences, the first one is a conscious decision, but the second one should not happen as long as you have the "presence" plugin enabled (it's enabled by default).

Post.new(title_ja: "").title
=> nil

This is what I get with a model like this:

class Post < ApplicationRecord
  extend Mobility
  translates :title, backend: :table, locale_accessors: true
end

Did you set the default_options configuration to a new hash? If you did and did not include presence: true then that would explain the behaviour you see.

If you want to just override specific option values, rather than the entire hash, you should set each value on its own:

Mobility.configure do |config|
  # ...

  config.default_options[:locale_accessors] = true
  config.default_options[:type] = :string
end

@shioyama
Copy link
Owner Author

shioyama commented Sep 14, 2017

About versions, I'll release cf3860f as 0.2.3 and point to it from a new branch 0.2, and change the version on master to 0.3.alpha`. I'm going to wait for a few more things (including #84 and possibly an implementation for #90) before releasing 0.3.0.

@shioyama
Copy link
Owner Author

The other thing missing from Globalize compatibility (which is harder to implement) is #51.

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.

2 participants