I18n support #4

Closed
wants to merge 9 commits into from

4 participants

@liff

Here's a proposed way to deal with i18n. Summary of the changes:

  • to_select uses translated values, if available,
  • add method EnumClass.translate for looking up translated values,
  • add :translation key to the attribute method.

The changes should be backwards compatible except for the attribute method's :translation key, which could conflict with metadata lookup.

Pull if it looks ok to you.

@adzap
Owner

It looks really good. Is it possible to use the I18n :default option instead of a rescue to set the value when there is no translation?

@liff

That's certainly a better way to do it.

@liff

I changed it to use :default, see commit above.

@adzap
Owner

Cool.

I think the I18n yml file key should be under its own top level key of active_enum. Also allowing per attribute, per model definitions suggests that a different value for the enum could be returned dependent on which attribute it was enumerating. I realise you can have a enum that is only defined for one attribute, but the I18n implementation has to be consistent with having a standalone enum that enumerates one or many attributes across models.

I haven't given the I18n integration a great deal of thought before, so I just want to throw a few ideas around. I am not necessarily suggesting you implement, just discuss them.

The other thing I am wondering is whether this could be integrated as a plugin storage engine. Currently only a memory storage engine is implemented. But you could do a I18n which did a more transparent look up behind the scenes, rather than the translate method. The engine could just extend the memory engine to do a I18n look up or return the value from the original enum definition on a miss.

Let me know what you think.

@liff

The choice of activerecord as the top level key was based on looking at some other enumeration implementations (simple_enum, symbolize). As ActiveEnum::Base has no ties to ActiveRecord, however, I suppose it makes more sense to put them under active_enum. I'll change that.

I'm not sure how the translation could be dependent on the attribute as the enum class itself has no knowledge of the attribute it's enumerating. It could be implemented in the attribute read method but to me user.sex(:to_select) doesn't seem quite right.

Can you elaborate on the storage engine idea? Would it return a translation instead of the enum's name?

@adzap
Owner

What I meant was that your example defines the top level Sex enum and the I18n yml has the mapping of model -> attribute -> enum values. This implies that you could have different values for the same enum when enumerating a different attribute.

For example, a another model and attribute using the enum, but defining the values different:

activerecord:
enums:
user:
sex:
male: elam
female: elamef

Probably just want
activeenum:
sex:
male: elam
female: elamef

Looking at the implementation now, I can see that model is not included in the scope, so it is really just an issue with the example.

The storage engine could be used in place of doing the explicit translation. Create a new I18n storage class and config ActiveEnum to use this storage engine instead. Then implement the storage engine as a subclass of the memory engine and do the translations in the get_by_name and values method. This removes the need to change the Base class and makes the translation automatic. See active_enum/storage/memory_store.rb as the example.

@liff

The model is included in the translation key if the enum is defined inside a model/class.

class Person
  enumerate :sex { ... }       # enum class is Person::Sex; keys under active_enum.person.sex
  enumerate :sex, :with => Sex # enum class is Sex; keys under active_enum.sex
end

The i18n documentation section could be clearer on that.

I'll look into the storage engine method.

@adzap
Owner

Thanks. I didn't realise what the behaviour of underscore on a class name in a module. I see now separates with the '/'.

@felixbuenemann

There seems to be no updates on this request. Any reason why it hasn't been merged yet?

@liff

I got distracted before finishing the engine method :-/ Need to look into it some day.

@Godisemo

Will this be implemented anytime soon?

@felixbuenemann

@liff Probably outlining what needs to be done would be helpful, so someone else could finish it.

@adzap
Owner

I still want to see if the engine method is possible. I will explore it more.

@adzap
Owner

I have implemented a I18n solution on master using a storage engine. You need to set ActiveEnum.storage = :i18n.

Checkout the specs and the generate the base locale yml with

rails g active_enum:locale

Let me know what you think

@adzap
Owner

I didn't get any feedback, but I've been using and it is working for me. So I will close this issue.

@adzap adzap closed this Jun 9, 2012
@felixbuenemann

@adzap I tried to replace the old I18n implementaton with your code, added "ActiveEnum.storage = :i18n" to the initializer and it seems to work fine. Thanks!

Are you planning on releasing a new version on rubygems anytime soon?

@adzap
Owner

Yes, very soon. I have been using it successfully. I think only need to add some documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment