Lazy translation and disabling string concatenation

[mateo murphy]

I've written some code to support both these features while retaining full backwards compatibility:

http://github.com/mateomurphy/rails/blob/73ac06a165e0e255bab03f693a6158c8dc67af17/activerecord/lib/active_record/validations.rb

All current tests pass as is, but I still have to write new tests specifically for the new functionality. There's also a bit of refactoring that I plan on doing; modifying the tests that expect generate_message to be called on errors.add, and moving the generation of full messages from Errors to Error.

As discussed previously, it would be possible to return the errors object rather than a string on calls like errors.on('foo'), and have the object act like a string via to_str and method_missing; I've tested it and it works in most cases (all the current tests pass), but could introduce bugs in edge cases. This might be a feature best left for 3.0...

Any thoughts?

[Masao Mutoh]

Hi,

Seems nice.

Could you avoid to check Symbol in Error#to_s ?

class Error
:
:
def to_s
@errors.generate_message(@attribute, @message, @options)
end
end

:
:
def generate_message(...)
if message.is_a?(Symbol) # Check it here.
:
else
key = message
end
I18n.translate(key, options)
end

This aims to be possible to implement gettext(or other i18n translation libs)
as a backend with no monkey patches.

--
Masao Mutoh <muto...@gmail.com>

[Masao Mutoh]

Hi,

Oops.
It's better to pass Same options in generate_message to I18n.translate.

On Fri, 15 May 2009 00:43:09 +0900
Masao Mutoh <muto...@gmail.com> wrote:

> Hi,
>
> Seems nice.
>
> Could you avoid to check Symbol in Error#to_s ?
>
> class Error
> :
> :
> def to_s
> @errors.generate_message(@attribute, @message, @options)
> end
> end
>
> :
> :
> def generate_message(...)
> if message.is_a?(Symbol) # Check it here.
> :
> else
> key = message
> end
> I18n.translate(key, options)
> end

def generate_message(...)
if message.is_a?(Symbol) # Check it here.
:
else
key = message
end

options = {
...
}.merge(options)
I18n.translate(key, options)
end

HTH,

>
> This aims to be possible to implement gettext(or other i18n translation libs)
> as a backend with no monkey patches.
>
> On Thu, 14 May 2009 11:21:47 -0400
> mateo murphy <mateo....@gmail.com> wrote:
>
> > I've written some code to support both these features while retaining full
> > backwards compatibility:
> > http://github.com/mateomurphy/rails/blob/73ac06a165e0e255bab03f693a6158c8dc67af17/activerecord/lib/active_record/validations.rb
> >
> > All current tests pass as is, but I still have to write new tests
> > specifically for the new functionality. There's also a bit of refactoring
> > that I plan on doing; modifying the tests that expect generate_message to be
> > called on errors.add, and moving the generation of full messages from Errors
> > to Error.
> >
> > As discussed previously, it would be possible to return the errors object
> > rather than a string on calls like errors.on('foo'), and have the object act
> > like a string via to_str and method_missing; I've tested it and it works in
> > most cases (all the current tests pass), but could introduce bugs in edge
> > cases. This might be a feature best left for 3.0...
> >
> > Any thoughts?
> >
> > > >
> >
>
>
> --
> Masao Mutoh <muto...@gmail.com>

--
Masao Mutoh <muto...@gmail.com>

[mateo murphy]

On Thu, May 14, 2009 at 11:43 AM, Masao Mutoh <muto...@gmail.com> wrote:

Hi,

Seems nice.

Could you avoid to check Symbol in Error#to_s ?

class Error
 :
 :
 def to_s
     @errors.generate_message(@attribute, @message, @options)
 end
end

 :
 :
 def generate_message(...)
    if message.is_a?(Symbol)    # Check it here.
      :
    else
      key = message
    end
    I18n.translate(key, options)
 end

This aims to be possible to implement gettext(or other i18n translation libs)
as a backend with no monkey patches.

Good idea, I think I'll need to pass in the string as default for compatibility though

[Sven Fuchs]

Hi Mateo

sorry, i've been off for a vacation for a week and afterwards been busy working through all the stuff that piled up meanwhile.

I agree extracting out an Error object makes a lot of sense and - going down that road - the refactorings you suggest seem obvious to me.

Obviously this might break some behaviour in 2.3.x (errors.on(:foo) now returns an Error instead of a String). So I wonder what to do with 2.3.x. 

Could we intermediary fix this by storing two Errors collections, one for the message, one for the full_message? Obviously this would be overly expensive, but at least we could propose an alternative for a) doing nothing about this in 2.3 and b) potentially breaking stuff for people.

Another path might be to change the implementation of Error a bit along these lines:

class Error < String # stores the message translation

  attr_reader :options

  def full_message

    I18n.t(:whatever, options)

  end

end

Or am I missing something? IIRC a similar approach was suggested a while ago by Erkki.

(Please bear with me if I'm missing stuff that was discussed previously. I'm trying to catch up and get back into this.)

[Lawrence Pit]

Hi All,

Just curious if there was any more progress on this front?

Mateo, was there a rails lighthouse ticket for that patch you wrote?

I think Sven's suggestion to make Error inherit from String to stay
backwards compatible makes sense.



Cheers,
Lawrence

[mateo murphy]

Hi All,

Sorry for the lack of news, I was sick for a while and then too busy catching up on work to work on this.

In any case, unless I'm missing something, having Error inherit from String means you don't get the benefit of lazy translation? Or is there a way to change the value of the string when it's used as a string?

I've yet to make a patch, as there's a fair number of tests that need to be (re)written. I'll have time to work on that this week though.

[Sven Fuchs]

On 28.06.2009, at 19:35, mateo murphy wrote:

In any case, unless I'm missing something, having Error inherit from String means you don't get the benefit of lazy translation? Or is there a way to change the value of the string when it's used as a string?

Unless I'm missing something it actually only makes sense for maintaining backwards compatibility to peoples code (tests most probably). I'd expect quite a lot of people to test for equality of error messages so we couldn't backport a change that introduces Error objects to Rails 2.3

So maybe the best road forward would be:

- for Rails 3 extract Error as you've outlined in your code

- for Rails 2.3 instead store the key on the returned translation (String) in the client code (i.e. in ActiveRecord) so that we can still look up the full messages later

This seems like the best compromise to me because

+ we can support the required behaviour in both cases

+ we don't break tons of tests for Rails 2.3

+ we can switch to the "right" implementation with Rails 3

- we need to do 2 lookups in Rails 2.3

This could then even accompanied with a plugin that monkeypatches Rails 2.3 the same way as Rails 3 then behaves (i.e. returning an Error object) in case people bitch about wasted resources.

wdyt?

[mateo murphy]

On Sun, Jul 5, 2009 at 4:48 PM, Sven Fuchs<sven...@artweb-design.de> wrote:
> On 28.06.2009, at 19:35, mateo murphy wrote:
>
> Unless I'm missing something it actually only makes sense for maintaining
> backwards compatibility to peoples code (tests most probably). I'd expect
> quite a lot of people to test for equality of error messages so we couldn't
> backport a change that introduces Error objects to Rails 2.3

Right, there is a bit of confusion. In the code I wrote, all methods
return strings as before; the Error object is simply used to hold on
to the various options, so that when the full_messages (for example)
is called, the result is in the current locale, rather than the locale
that was active when validation occurred. This approach retains full
backwards compatibility; none of the current tests break.

I agree that for 3.0, it would make sense to return the error objects,
but I was targeting 2.3 with my code.

[Sven Fuchs]

So, the confusion was probably on my part.

This sounds good to me. So, is there anything holding us back from
providing a patch?


On 06.07.2009, at 04:39, mateo murphy wrote:

>
> On Sun, Jul 5, 2009 at 4:48 PM, Sven Fuchs<svenfuchs@artweb-

[José Valim]

Definitely +1 for a patch. We could then talk with Josh Peek and ask
him to port those changes to Rails 3.0 on his ActiveModel fork.

[mateo murphy]

Mostly just some more tests, although there's a change I wanted to
make to generate messages that Masao suggested that I'm looking into.
I'll wrap that up over the next couple of days.

[Sven Fuchs]

Hey folks,

I want to get this sorted. As we agreed to only target 2.3.x for now
we'll still have to make sure this makes it way to ActiveModel in
time, so let's push a bit.

I've cherry-picked Mateo's excellent work and added a few refactorings
on top of it:

http://github.com/svenfuchs/rails/commits/2-3-stable

Tests pass. The refactoring should not break any tests that do not
rely on particular internal methods being called.

We now have

* an Error object that carries stuff around. It will be translated
when the user tries to access the message somehow.
* two separate translation key hierarchies that can be used separately
("messages" and "full_messages"). Both behave the same regarding STI
etc. and both receive translated model and attribute names.

I believe users should now have enough flexibility to cover even weird
usecases.

Can everybody interested in this stuff please give this a brief review?

[José Valim]

Sven,

Notice that one feature is missing after Mateo commits:

http://github.com/rails/rails/blob/cfd421daa2b04216e27d666361eb4053020e027d/activemodel/lib/active_model/errors.rb#L106

However, as we discussed on #rails-contrib, we might replace that
feature for one more powerful, like:

activerecord.errors.format: "\{{attribute}} \{{message}}"

Which would be the default format if no full_message key is available.

Congratulations to everyone, an excellent work has been done!

[Mateo Murphy]

Hi Sven,

Thanks for taking my changes and running with them, I'd gotten a
bogged down in some refactoring of generate_message and hadn't had
the time to finish what I wanted to. A couple of comments:

1) Memoizing message in Error means that you can't change locales
after the first call to message, which was the main reason for the
creation of the Error object in the first place.

2) Having two seperate translation hierarchies is a great idea, but I
can't figure out how this actual works in your latest commit, and I
don't see any tests for it either...?

3) As for line 32, in activerecord/lib/active_record/validations.rb,
it's because messages on base don't have a related attribute, so the
full_messages should be the same as the message.

Other than that, looks good! Thanks again

[Simone Carletti]

On Wed, Aug 5, 2009 at 8:11 PM, Sven Fuchs <sven...@artweb-design.de> wrote:

Can everybody interested in this stuff please give this a brief review?

I would be really interested, but unfortunately it's a really busy period for me. I hope to get more time soon. :)

--
Simone Carletti

Site & Blog: http://www.simonecarletti.com
Email: wep...@weppos.net
LinkedIn: http://linkedin.com/in/weppos
Nick: weppos | Skype: weppos

[Sven Fuchs]

Hi Mateo,

On 06.08.2009, at 03:19, Mateo Murphy wrote:
> 1) Memoizing message in Error means that you can't change locales
> after the first call to message, which was the main reason for the
> creation of the Error object in the first place.

good point.

> 2) Having two seperate translation hierarchies is a great idea, but I
> can't figure out how this actual works in your latest commit, and I
> don't see any tests for it either...?

Wow, your right. It's still been sticking around on a local branch
which I forgot to merge. Also, I probably got something wrong here. We
currently now have this defaults hierarchie for messages:

active_record.errors.models.topic.attributes.title.blank
active_record.errors.models.topic.blank
active_record.errors.messages.blank

Can anybody remind me why this is not

active_record.errors.messages.models.topic.attributes.title.blank
active_record.errors.messages.models.topic.blank
active_record.errors.messages.blank

??

Because I think *if* we do a separate hierarchie for full_messages it
should look similar to this:

active_record.errors.full_messages.models.topic.attributes.title.blank
active_record.errors.full_messages.models.topic.blank
active_record.errors.full_messages.blank
active_record.errors.full_messages.default # :en => "{{attribute}}:
{{message}}"

Obviously these hierarchies would then be asymetric - which probably
will cause tons of confusion.

So, should we really do this in 2.3.x? Or should we just use
active_record.errors.full_messages.default ("{{attribute}}:
{{message}}" for :en) and change the standard message hierarchie in 3.0?

Any opinions?

> 3) As for line 32, in activerecord/lib/active_record/validations.rb,
> it's because messages on base don't have a related attribute, so the
> full_messages should be the same as the message.

Gotcha :)

Cheers,
Sven

[Mateo Murphy]

On 7-Aug-09, at 4:14 AM, Sven Fuchs wrote:

> [...] I probably got something wrong here. We

> currently now have this defaults hierarchie for messages:
>
> active_record.errors.models.topic.attributes.title.blank
> active_record.errors.models.topic.blank
> active_record.errors.messages.blank
>
> Can anybody remind me why this is not
>
> active_record.errors.messages.models.topic.attributes.title.blank
> active_record.errors.messages.models.topic.blank
> active_record.errors.messages.blank
>
> ??
>
> Because I think *if* we do a separate hierarchie for full_messages it
> should look similar to this:
>
> active_record.errors.full_messages.models.topic.attributes.title.blank
> active_record.errors.full_messages.models.topic.blank
> active_record.errors.full_messages.blank
> active_record.errors.full_messages.default # :en => "{{attribute}}:
> {{message}}"
>
> Obviously these hierarchies would then be asymetric - which probably
> will cause tons of confusion.
>
> So, should we really do this in 2.3.x? Or should we just use
> active_record.errors.full_messages.default ("{{attribute}}:
> {{message}}" for :en) and change the standard message hierarchie in
> 3.0?
>
> Any opinions?

We could also support both the current and the ideal hierarchy, and
deprecate the current one for 3.0. This would mean extra code, but it
would allow people to transition from on to the other pretty smoothly.

Also, as I mentioned in passing before, I think generate_message
could really use some cleaning up in general; the handling of message
vs options[:default] is weird and unintuitive, and only attempts to
translate symbols (precluding the use of strings and gettext as a
translation backend). Maybe we can take a look at this once you've
committed your other code

[Mateo Murphy]

To expand on my previous email, here are the issues I've run into
regarding generate_message:

When you add a validation to a model, such as

validates_acceptance_of :eula, :message => "must be abided"

The documentation states the :message is a custom error message, and
that the default is "must be accepted". However, what actually
happens is that :accepted is stored as the message, and the :message
portion is stored in the options as :default.

What this means is that when generate message gets called, it gets
called with

generate_message(:eula, :accepted, :default => 'must be abided')

Which is the opposite of what you'd expect, and which is why the
method swaps 'message' and 'options[:default]'. But this only happens
if 'options[:default]' is a symbol; if it's a string, the
translations for 'message' will be looked up before 'options
[:default:]', which is probably not what you'd expect based on the
declaration of the validation.

The other issue is that generate_message is only called when
'message' is a symbol; which renders the whole thing incompatible
with I18n systems like get text, that rely on keys for translation.

So, my thoughts regarding this:

1) The custom error message should not be stored as options
[:default]; changing this to options[:custom] would make the
internals a lot easier to understand

2) generate_message should either be called as

generate_message(:eula, :accepted, :custom => 'must be abided')

or

generate_message(:eula, 'must be abided', :default => :accepted)

And the internals adjusted accordingly. I think the 2nd makes more
sense in terms of interface, but would make the calling code a bit
more complex

3) generate_message should be designed to handle strings as I18n keys
for both the default and the custom message.

Any thoughts on all of this?

[Sven Fuchs]

Hi Mateo,

I agree that the current generate_message implementation is highly
questionable. I guess that's mostly because we've only refactored as
much as needed to get things (more or less) working last year.

In my current local branch (will push later) I've changed things so that

1) validates_* methods do not use the default option at all
2) generate_message only interpolates and then immediately returns
when the given message is a String

Thus people can use either of

class Article < ActiveRecord::Base
validates_presence_of :title, :message => "the message"
validates_presence_of :title, :message => :the_message
end

The first one allows you to skip translations and easily define
messages in plain text (legacy behaviour). The second one allows you
to define a key that will be used in favor of the default key for this
validation (:blank in this case).

I've currently done it so that #validates_* only use (through #add)

generate_message(:title, :blank)
generate_message(:title, :the_message) and
generate_message(:title, 'the message')

depending what's defined through the class macro.

I guess this is still not perfect since the Error object does not know
what "kind" of error it is. Otoh error keys are reused and thus do not
map perfectly to validation macros anyway, so maybe this is not a
problem at all.

[Mateo Murphy]

On 10-Aug-09, at 3:11 AM, Sven Fuchs wrote:

Hi Mateo,

I agree that the current generate_message implementation is highly  
questionable. I guess that's mostly because we've only refactored as  
much as needed to get things (more or less) working last year.

In my current local branch (will push later) I've changed things so that

1) validates_* methods do not use the default option at all

Right, that makes perfect sense

2) generate_message only interpolates and then immediately returns  
when the given message is a String

Thus people can use either of

class Article < ActiveRecord::Base
  validates_presence_of :title, :message => "the message"
  validates_presence_of :title, :message => :the_message
end

The first one allows you to skip translations and easily define  
messages in plain text (legacy behaviour). The second one allows you  
to define a key that will be used in favor of the default key for this  
validation (:blank in this case).

I've currently done it so that #validates_* only use (through #add)

generate_message(:title, :blank)
generate_message(:title, :the_message) and
generate_message(:title, 'the message')

depending what's defined through the class macro.

Right, the thing is, if you're using gettext, your lookup keys are strings as well, so I don't think it's best to assume that string == plain text; I think we should run them through I18n as well, rather than simply interpolating them.

I guess this is still not perfect since the Error object does not know  
what "kind" of error it is. Otoh error keys are reused and thus do not  
map perfectly to validation macros anyway, so maybe this is not a  
problem at all.

Not to mention that it's possible to add errors directly to the Errors object anyway

[Lawrence Pit]

Great work.

I'd rather like to see the +each+ method +yield attr, error+ instead of
+yield attr, error.to_s+

generate_full_message misses doc... what's the difference between
+message+ and +full_message+ ?

when the I18n.t method is called in generate_message and
generate_full_message, should it not do a reverse_merge on the options
hash? As it is it overrides the :scope option; plugins may want to use
different scopes.



Cheers,
Lawrence

[Lawrence Pit]

Mateo Murphy wrote:
> 1) Memoizing message in Error means that you can't change locales
> after the first call to message, which was the main reason for the
> creation of the Error object in the first place.
>

For me the main reason to see the creation of the Error object was to
allow lazy translations: I don't want AR to do the translations, so I
wouldn't even call the message method.

Cheers,
Lawrence

[Sven Fuchs]

Hey Lawrence,

On 27.08.2009, at 06:57, Lawrence Pit wrote:
> I'd rather like to see the +each+ method +yield attr, error+ instead
> of
> +yield attr, error.to_s+

That would break existing behaviour though. So I believe we can't do
it in 2.3.x .

We're only talking about 2.3.x here btw. :)

> generate_full_message misses doc... what's the difference between
> +message+ and +full_message+ ?

Good point. I'll add some docs.

full_message does not go through the same key hierarchie as message
does. Instead you can only specify "wrapper" or "format" strings that
will wrap the message. This is certainly not an ideal solution, but it
should be sufficient for 2.3.x (where you currently can not customize
the full_message format at all).

> when the I18n.t method is called in generate_message and
> generate_full_message, should it not do a reverse_merge on the options
> hash? As it is it overrides the :scope option; plugins may want to use
> different scopes.

Yes, José already beat me on that one. I've changed it (maybe not
pushed, yet).

Thanks!

Sven

[José Valim]

Awesome work Sven!

> > when the I18n.t method is called in generate_message and
> > generate_full_message, should it not do a reverse_merge on the options
> > hash? As it is it overrides the :scope option; plugins may want to use
> > different scopes.

Yep, already provided a patch for that! ;)

[Lawrence Pit]

I'd rather like to see the +each+ method +yield attr, error+ instead  
of
+yield attr, error.to_s+
    

That would break existing behaviour though. So I believe  we can't do  
it in 2.3.x .
  

I noticed the presence of a new +each_error+ method, awesome! :-)


Question: when I do model.add(:foo, "some error message") and then later do model.errors[:foo] it will go through the +generate_message+ method of the Error object, and hence go through a I18n.translate call. Is that wanted behaviour? Shouldn't it always simply return the hard-coded message string?




Best regards,
Lawrence

[Sven Fuchs]

> I noticed the presence of a new +each_error+ method, awesome! :-)

Yeah, that one's especially for you ;)

> Question: when I do model.add(:foo, "some error message") and then
> later do model.errors[:foo] it will go through the +generate_message
> + method of the Error object, and hence go through a I18n.translate
> call. Is that wanted behaviour? Shouldn't it always simply return
> the hard-coded message string?

Yes, it's intended. Mateo pointed out that Gettext people will expect
their String keys to be translated here. Unless you set any
translation for these keys it should return the original strings though.