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

MONGOID-4052 Add option to return all translations when calling #only for a field #4277

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@estolfo
Copy link
Contributor

commented Apr 26, 2016

No description provided.

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

@estolfo on second thought here, for only why not make localize = false the behavior always. Is there any real meaning to have localize true?

At least localize = false should be the default, even if we keep localize = true for backwards compatibility (though I doubt anyone would use it in practice)

If a user really wants a particular locale they should be able to do field.en for example.

@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2016

I think users expect localizations to be applied, if a field is localized and there is a locale set. I'd like to keep it with { localize: true } as the default and consider { localize: false } the exception. This is consistent with accessing the attribute after doing a simple #where:

I18n.locale = :de
d = Dictionary.first
d.description
=> "deutsch-text"
@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

I really don't think so, because 90%+ of users using I18n have fallbacks enabled, i.e. if key is missing for :de, fallback to the :en word. The major problem with Mongoid here is that it doesn't work with fallbacks. Unless a key was set for the specific locale it doesn't work.

Fallbacks are especially critical for db-stored (e.g. user-entered) data, because users are lazy and don't enter all locales.

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

Also, note that if we set localize = false your example will still work exactly as you have it for .only method:

I18n.locale = :de
d = Dictionary.only(:description, localize: false).first
d.description
=> "deutsch-text"
I18n.locale = :en
d.description
=> "english-text"
d.description_translations
=> {en: "english-text", de: "deutsch-text"}     # GOOD!

I18n.locale = :de
x = Dictionary.only(:description, localize: true).first
x.description_translations
=> {de: "deutsch-text"}     # BAD -- :en has disappeared!
@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

The only use case for localize: true is the very rare case where you only want a specific locale and want to ingore all other locales to reduce your data payload. As mentioned you can achieve the same result by simply doing .only("description.de") or .only("description.#{I18n.locale}"). Going by the principle of least surprise the default should definitely be localize: false, i.e. pull all locales from the database.

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

(apologies for bombarding with text but one more point...) localize: true/false is a misnomer here. In fact localize: false really means "load all locales and display the correct one on the frontend, using fallbacks if necessary", where as localize: true means "load only currently locale, and the frontend will return nil if you switch locales or try to fallback"

@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2016

Hi @johnnyshields

We have two questions here:

What should the default be?
I don't think we should load all translations from the database by default. Users will use #only in order to do a projection -- i.e. select the exact fields fetched for each document. The list of translations could potentially be long and I'd rather not load all of them, without the user explicitly asking to do so. In using #only, users will be aware that they are only pulling a single field and I think it's appropriate that users will have to opt-in for loading all translations.

What should the option be called?
I agree the option name is misleading. The behavior difference between localize: true and localize: false is whether fallbacks are possible, so maybe we can call it "fallbacks". The behavior would then be:

I18n.locale = :de
x = Dictionary.only(:description).first
x.description_translations
=> {de: "deutsch-text"}

I18n.locale = :de
d = Dictionary.only(:description, fallbacks: true).first
d.description
=> "deutsch-text"
I18n.locale = :en
d.description
=> "english-text"
d.description_translations
=> {en: "english-text", de: "deutsch-text"} 

I18n.locale = :de
x = Dictionary.only(:description, fallbacks: false).first
x.description_translations
=> {de: "deutsch-text"}

What do you think?

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented May 2, 2016

@estolfo I think:

  1. Dictionary.only(:description) should load all translations. You can then call d.description to get the current locale, or d.description_translations to see the full hash. This is the most intuitive; if you think about the database structure, you would expect this to fetch the full hash without additional magic.
  2. If you want a particular locale, you can do Dictionary.only(:description.de)
  3. There does not need to be a fallback option, as (1) and (2) cover all use cases.
@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented May 5, 2016

@estolfo any thoughts on the above?

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented May 18, 2016

@estolfo pinging on this

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2016

@estolfo pinging again...

@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2016

Hi @johnnyshields
I'm just getting back to this and will have an update soon. Thanks for your patience!

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

@estolfo any update on this?

@estolfo estolfo force-pushed the estolfo:MONGOID-4052-localized-only branch from b4e37de to f3065e5 Jul 1, 2016

@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

Hi @johnnyshields
I've updated the code to return all translations by default but also allow a :localize option passed to #only for achieving the previous behavior.
Let me know what you think, I believe this is the behavior you've been advocating for.

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

@estolfo firstly thank you for your support on this. I agree with your implementation.

Two comments:

1) My ideal would be that we do not need "localize" at all. The following two should be equivalent:

Dictionary.only(:description, localize: true) ==
Dictionary.only("description.#{I18n.locale}")

This is quite a rare case so in my opinion the second way is OK; we don't need to further complicate Mongoid code. That being said I don't have a strong opposition to the :localize option.

2) How does .pluck behave? In my opinion it should return the raw translation hash, e.g.:

Dictionary.pluck(:description)   #=> { en: "Very good!", de: "Sehr gut!", ja: "すばらしい!" }
@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

Hi @johnnyshields

Dictionary.only(:description, localize: true)
Dictionary.only("description.#{I18n.locale}")

are equivalent with this PR. Have you tested and found otherwise?

We need the localize option to be passed around internally anyway, as the code is written to apply the localization if the field is localized. I decided I might as well expose the option in the public API for #only, as I'm going to have to pass it through the code path anyway.

I haven't looked at #pluck yet. I'll do that now.

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

@estolfo ok agreed on #1 then. Fine to have localize option.

@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

#pluck returns all translations

@estolfo estolfo force-pushed the estolfo:MONGOID-4052-localized-only branch from f3065e5 to f78b26d Jul 1, 2016

def normalized_key(name, serializer)
serializer && serializer.localized? ? "#{name}.#{::I18n.locale}" : name
def normalized_key(name, serializer, opts = {})
if serializer && serializer.localized? && opts[:localize] != false

This comment has been minimized.

Copy link
@johnnyshields

johnnyshields Jul 1, 2016

Contributor

shouldn't this be && opts[:localize] (i.e. both nil and false skip localization?)

This comment has been minimized.

Copy link
@estolfo

estolfo Jul 4, 2016

Author Contributor

No, because if it's nil, this condition should be true so that other behavior is not affected. Other code paths use this method and don't expose a :localize option.

This comment has been minimized.

Copy link
@johnnyshields

johnnyshields Jul 4, 2016

Contributor

I strongly believe we should change the default behavior, and announce it as a breaking change. It can be a separate PR if needed. The :localize option is almost never what anyone would want to use, you'd only want to use it in explicit performance optimization scenarios.

This comment has been minimized.

Copy link
@johnnyshields

johnnyshields Jul 4, 2016

Contributor

...sigh. This goes back to my previous statement that :localize itself is a misnomer. It should really be called only_current_locale: true or something like that, because without it the field will still be localized--in fact more so because it can use fallbacks.

This comment has been minimized.

Copy link
@johnnyshields

johnnyshields Jul 4, 2016

Contributor

Reading my earlier comments. Rather than localize: true we should call it something like:

  • i18n_fallbacks: false
  • include_all_locales: false
  • only_current_locale: true

...or better yet, remove this option entirely as I suggested earlier.

else
opts = {}
args = args.flatten
end

This comment has been minimized.

Copy link
@johnnyshields

johnnyshields Jul 1, 2016

Contributor

this could be refactored to use extract_options from active support

This comment has been minimized.

Copy link
@estolfo

estolfo Jul 4, 2016

Author Contributor

I didn't know about that method, thanks!

@estolfo estolfo force-pushed the estolfo:MONGOID-4052-localized-only branch from 688abd6 to 4dacb86 Jul 4, 2016

@estolfo estolfo force-pushed the estolfo:MONGOID-4052-localized-only branch from 4dacb86 to 7f04cb4 Jul 4, 2016

@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2016

Would you like to open a pull request implementing this how you'd like? That might end up being more efficient than going back and forth = )

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2016

Sure, I'd be glad to!

@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2016

Awesome, that seems like a much easier option and I want to make sure this works how you've requested.

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2016

Thanks! I'm a heavy user of Mongoid's I18n functionality.

@simi

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2016

@estolfo is this last blocker for 6.0 release? Do you need help here?

@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2016

Hi @simi
These are the remaining tickets holding up the release of 6.0.

I've been in touch with the JRuby team to figure out why performance has decreased so drastically on JRuby. Here is my ticket with them.

I'd welcome help on implementing this PR, as I've had to turn my attention to the driver in the last few weeks and a PR would help speed up the release of 6.0. Also, if you'd like to help investigate why performance is so bad on JRuby, that'd be great as well!

Thanks = )

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

@simi @estolfo I will help with this ticket over the weekend and get it done

@estolfo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2016

awesome! I'll make some time for it over the next two days as well. Sorry for the delay, but we really need to figure out the JRuby issue too...

@johnnyshields

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

@estolfo @simi I've raised #4299

@estolfo estolfo closed this Jul 21, 2016

@estolfo estolfo deleted the estolfo:MONGOID-4052-localized-only branch Dec 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.