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

Return locale from backend read/write methods #305

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shioyama
Copy link
Owner

@shioyama shioyama commented Nov 6, 2018

This is a major API change coming for v1.0. Backends will now return the locale and value from read and write methods. This allows for better handling when using things like fallbacks, e.g. see the issue here.

This will not change the normal model getter/setter methods, so most applications will be unaffected.

@shioyama shioyama force-pushed the return_locale_from_backends branch 2 times, most recently from ed9fa9b to 6241068 Compare November 7, 2018 14:05
@shioyama
Copy link
Owner Author

shioyama commented Nov 7, 2018

@pwim Interested to get your thoughts on this. I'm making an api change for a v1.0 release that I'm planning somewhere in the next month or so.

The change is that backends will now return the locale along with the value from a read (and write mainly for consistency). So e.g. if you're using fallbacks, this would give you a way to know which locale the returned translation is actually in. This is something that has been asked for, but also I think it's something that makes sense to have available, since unless Mobility provides this info, there is really no consistent way to get at it.

Attribute getters/setters would not change (obviously), so I believe most users would be unaffected, but I'm hesitating about how to return the locale. The code in this PR returns a pair: [locale, value], so e.g. [:en, "foo"]. You can then destructure them:

locale, value = post.title_backend.read(:en)

I originally was going to return [value, locale], but looking through the code locale in read and write the locale is first (value is the second argument for write) so I thought this order would be more consistent.

Any thoughts?

@shioyama shioyama changed the title [WIP] Return locale from backend read/write methods Return locale from backend read/write methods Nov 7, 2018
@pwim
Copy link
Collaborator

pwim commented Nov 8, 2018

What the person on StackOverflow wants to do seems like an edge case. I'd want to better understand how they are using the "Content-Language" header before proceeding.

That being said, if you are going forward with exposing the language of the translation, I'd consider if it makes sense for read to return an object instead. For example, it could return something like a Mobility::Translation, which has attributes locale and value. This approach would still allow the getters and setters remain unchanged.

@shioyama
Copy link
Owner Author

shioyama commented Nov 8, 2018

I'd want to better understand how they are using the "Content-Language" header before proceeding.

That particular use case is not really so important. The use case for "I want to know if this is a fallback or the original translation" is pretty clear. I myself have needed that, for example showing a value in an interface which allows you to translate them: if it's not available, show the fallback but indicate it has not yet been translated. But you currently can't do that with Mobility.

Other plugins could also use this, e.g. knowing whether a value is the default or the actual translation, if a value was cached or not (for debugging), etc.

That being said, if you are going forward with exposing the language of the translation, I'd consider if it makes sense for read to return an object instead.

Yeah I thought of that. There is actually already a similar object:

Translation = Struct.new(:backend, :locale) do
%w[read write].each do |accessor|
define_method accessor do |*args|
backend.send(accessor, locale, *args)
end
end
end

I agree that might work, my concern there would be that plugins would have to modify the object passed back to them, and not sure how clean that would be. But then again, the changes with destructuring arrays in this PR are not so clean either...

@pwim
Copy link
Collaborator

pwim commented Nov 8, 2018

I myself have needed that, for example showing a value in an interface which allows you to translate them.

This makes sense. Though I don't think it specifically requires knowing what fallback is used, just whether or not their is a translation for a given locale.

I agree that might work, my concern there would be that plugins would have to modify the object passed back to them, and not sure how clean that would be.

Perhaps if I saw an example of how a plugin would use it, I could give better feedback, but I'm missing how a plugin would need to treat a translation object differently than an array. Rather than modifying the object itself, it should just be able to create a new instance instead (as presumably it would be doing if it worked with arrays).

@shioyama
Copy link
Owner Author

shioyama commented Dec 4, 2018

Though I don't think it specifically requires knowing what fallback is used, just whether or not their is a translation for a given locale.

Yes, but if we're going to return a boolean, we might as well return the actual locale, since this is actually simpler. Users can then do what they want with it.

Rather than modifying the object itself, it should just be able to create a new instance instead

Yes true. I'm still thinking about how to implement this, but if it's in it should be in for 1.0 since it's a major API change.

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