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

Add extension to find translations in a locale for Table backend #202

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

shioyama
Copy link
Owner

@shioyama shioyama commented Apr 10, 2018

This may also be useful for KeyValue backend, but for now just adding it for the Table backend. Now you can call:

post.translations.in(:en)

... and you get all translations in that locale. Currently, you'd need to do this:

post.translations.find { |t| t.locale == "en" }

which is kind of annoying, especially if you do it frequently. I also refactored the backend to use this extension method.

@shioyama
Copy link
Owner Author

I debated about naming, either for or in. Currently, translation_for on the backend returns a translation or builds a blank one if none exists. The extension method in here returns nil if none exist. We could have an option to build if one is missing like Globalize does... but I think, in the interest of simplicity, for now it will just take a locale and return a translation or nil.

@shioyama
Copy link
Owner Author

@pwim would this change be useful to you? I found it useful fixing an issue in refinery/refinerycms#3360 Wondering about the method name. in is short and sweet.

@pwim
Copy link
Collaborator

pwim commented Apr 10, 2018

This isn't something I'd use personally.

Generally speaking, I'd avoid generic method names like for and in, because of the risk of them clashing with other methods. Given that this method is likely only going to be used for edge case situations, I would have gone with something like in_locale.

@shioyama
Copy link
Owner Author

Yes, I'd actually considered in_locale. I justified the choice of in to myself by the fact that this method is only applied to translations, not to the model or anything else. So if there was a conflict, it would either be with ActiveRecord itself, or with something the application was doing on translations already.

That said though, I could imagine AR using a method like in in the future...

@shioyama shioyama mentioned this pull request Apr 10, 2018
@shioyama
Copy link
Owner Author

Decided it's safer to go with in_locale, as suggested. Thanks @pwim for quick feedback, always nice to get your opinion on these things.

@shioyama shioyama deleted the table_finder_extension branch April 10, 2018 09:05
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.

None yet

2 participants