This project is archived and is in readonly mode.

#3768 open
Rodrigo Rosenfeld Rosas

Add :full_message option to validations

Reported by Rodrigo Rosenfeld Rosas | January 21st, 2010 @ 08:11 PM | in 3.1

Follows a patch to add a full_message option to validations.

Currently Rails don't allow full control over validation messages without making use of I18n and even that solution is overkill when you only need to have full control over a validation message.

This patch allow validations to be written as:

validates_presence_of :name, :full_message => 'Please, specify your full name.'

Comments and changes to this ticket

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas January 21st, 2010 @ 08:17 PM

    There is no documentation nor test files in the patch because I need some help about how to write them.

    I've asked for instructions in the topic below but got no answer yet:
    http://groups.google.com/group/rubyonrails-core/msg/041076c49fcaccac

    Here is an extract:

    I don't know how to proceed with documentation, since it is not very DRY...

    Should I send a description to appreciation and then copy and paste it on every validation method?

    If so, would the following description be ok?

    "Configuration options: * message - A custom error message (default is: "can't be blank"). full_message can be specified instead, in which case the humanized attribute name won't be prepended to the message.
    You have to choose between either message or full_message, not both. If that happens,
    full_message takes precedence.
    ..."

    The only difference between validations would be the default...

    Regarding test writing, I would also ask for some guidance on it.

    I've noted that not all options are tested on each validation respective test.

    So, I would like to know if I should add a test on with_validations_test.rb,
    add a new test file or add a new test for every validation class.

    Please, how should I proceed?

    Could you help me with documentation and test instructions?

  • José Valim

    José Valim February 24th, 2010 @ 07:33 AM

    • State changed from “new” to “incomplete”

    Hey mate, if you need help to get it documented and tests, please ask in the mailing list. The patch needs to be improved in a sense that ErrorMessage should behave like a string, so self.errors[:attribute] always return strings and not mixed objects.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 24th, 2010 @ 10:30 AM

    Valim, I have first tried the list, as you can see in the link I've posted in my first comment, but I didn't get any help there.

    I had already discussed the issue you are talking about on the list. The problem is that I can't extend an String in a way that makes sense. I first tried ErrorMessage to extend String, but it didn't work as I was expecting... The mixed object was the only thing I could think of for not breaking API compatibility.

    For instance, if self.errors[:attribute] always return a string, how errors.full_messages would know if it has to include a prefix or not? If it returns an ErrorMessage object, it would break the API and possible a lot of plugins. The solution I proposed, although not ideal, is less invasive to current API.

    The correct way to do this in my opinion is to change the API so that errors[:attribute] would return an ErrorMessage object instead of a String, but that would be a big change. I could write such patch but I doubt it would be accepted as the core team doesn't seem to be interested in such a ":full_message" feature...

  • José Valim

    José Valim February 24th, 2010 @ 10:34 AM

    Something like this would work:

    class ErrorMessage < String

    def initialize(message, full_message=false)
  @full_message = full_message
  super(message)
end

def full_message!
  @full_message = true
end

def full_message?
  @full_message
end

    end

    And then in full error messages:

    each do |message|

    if message.respond_to?(:full_message?) && message.full_message?
  array << message
else
  # what we do today
end

    end

    I cannot see any drawbacks in this solution. Or am I missing something?

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 24th, 2010 @ 02:33 PM

    You're right. When I tested subclassing String, I probably missed the message parameter in initialize.

    But it is still a hack in the sense that unexpected things can happen (it would also happen with the current patch). For instance:

    error = ErrorMessage.new('test', true) error.sub(/t/,'a').full_message? # returns nil, although error.sub(/t/,'a').class == ErrorMessage

    That is just an example for sub, but other methods are affected too.

    But I think that is OK. At least it is better than the current patch. I'll update the patch to subclass String instead of mixed objects and will update this ticket than. Probably tonight when I have some time... Changing Rails API would be more elegant but much more intrusive too.

    Thanks!

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 25th, 2010 @ 03:34 AM

    Here is a new patch (still lacking documentation and tests) using your suggested approach.

    Should I post to rails core list again asking for help on documentation and tests?

  • José Valim

    José Valim February 25th, 2010 @ 06:24 AM

    Sure, without tests and/or documentation is quite hard to understand some pieces of the code. For example, why are you checking inside ErrorMessage if it's receiving another ErrorMessage? What is the use case? Why not use duck typing?

    Also, I noticed that you changed all validators. Would be better if we encapsulate the logic in one method in the EachValidator and DRY the +record.errors.add+ logic.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 25th, 2010 @ 12:00 PM

    I have dryied the code. The part I have changed in all validators were necessary because it was not modular, so I couldn't change just one place. See an example:

    # record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => options[:message])
  record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => message)

# message is a new method from Validator:
def message
  return nil unless options[:full_message] || options[:message]
  @message ||= Errors::ErrorMessage.new(options)
end

    How could I not change every validator once they extract the :message value from options?

    Supporting full_messages is not the only goal of this patch. Now plugins are able to do more interesting things with the params passed to validations.

    For instance:

      # show this specific message in blue if this validation fails
  validates_presence_of :field, :show_error => :blue

    All options would be available through error.message[:field].first.options, and it is also possible to know if a symbol was passed to the message evaluating what os passed for :full_message or :message with error.message[:field].first.message.

    Having said that, duck typing in ErrorMessage#initialize would loose the options and original message if another ErrorMessage is passed to ErrorMessage#new. The above extract of the patch wouldn't be necessary, but it is there just for performance concerns:

      predicate = ErrorMessage.new(predicate) unless predicate.is_a? ErrorMessage
  # this could be just:
  predicate = ErrorMessage.new(predicate)

    Regarding the documentation, it would be like the text in my first comment. I just don't know if I should add that explanation to every single validation...

    The test case would be something like (I didn't test the code below):

      test "Validation should accept an :full_message option" do
    fm = 'You should specify your full name.'
    # some setup code (validates_presence_of :name, :full_message => fm)
    assert_equals fm, errors.full_messages[:name]
  end
  # one more test for passing a symbol instead of a string to :full_message

    Don't hesitate in asking me further doubts.

    P.S: one more thing: with this change of subclassing String, now it is possible to use Array(messages) as in the original code instead of Array.wrap(messages). I just forgot to change this back on my patch, but I'll do it on next patch.

  • José Valim

    José Valim February 25th, 2010 @ 12:15 PM

    Yes, you need to change all validators but if you are going to do that, it's better that they now all call the same method. However, validators should not know about ErrorMessage, they should simple call record.errors.add which will know what to do.

    Also, I don't see the point in having options inside ErrorMessage. Don't try to embrace the world in this first patch (like adding extra options to validations), just ensure :full_message works. It makes hard to review and hides the real intent. After we get full_messages patch applied, we can continue from it.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 25th, 2010 @ 01:48 PM

    Sorry, I didn't understand. How much dryier can this be? All validators just call the message method from Validator (from which them all inherit from). That method extract the message from the options. What would suggest instead of this? How could this be simpler than that?

    And I don't get what are the advantages for not storing all validation options? Why should this go in a separate patch?

    I'm not hiding the real intent, but I do intend multiple features:
    - supporting full_messages - store all validation options

    If I have some way to know what options were passed to some validation, I could have done a plugin without monkey patches for supporting full_messages, for instance... This change will be specially useful for I18n plugins because they will be able to do lazy evaluation (actually re-evaluation) for instance if they decide to override full_messages_for, for instance.

    The patch for supporting just the :full_message option would not be that different from what it is now. Why do you think this is difficult to review? I'll add all this documentation (including custom options) in the final patch.

  • José Valim

    José Valim February 25th, 2010 @ 04:14 PM

    This is exactly the issue. If you want to add multiple features, do them in separate patches and not in only one.

    And is not only about being dry, validators should not know anything about ErrorMessage. ErrorMessage is an ActiveModel::Errors concern, not a ActiveModel::Validators one. Everything on the validators should be done through the errors.add interface.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 25th, 2010 @ 04:55 PM

    This is exactly the issue. If you want to add multiple features, do them in separate patches and not in only one.

    I would agree if they were two completely and independent things, but I think they are too closely related in terms of implementation that I don't understand why would it be better to separate in two patches... The final review effort will actually be higher... Splitting it in two patches would require me a lot of effort and I would just want to understand why it does compensate... I mean, what are the disadvantages of storing the options passed to the validators alongside with the message?

    I can't understand why you are insisting that validators should not know about ErrorMessage... There is no mention to ErrorMessage in any of the validators... The only difference is that instead of passing options[:message] to errors.add, they would pass the result of the method 'message' from their parent Validator. If they wish to pass options[:message] instead, it will still work as before, but will not support the full_message parameter... Could you please be more specific about your concerns?

  • José Valim

    José Valim February 25th, 2010 @ 05:05 PM

    You wrote previously:

    # message is a new method from Validator:
def message
  return nil unless options[:full_message] || options[:message]
  @message ||= Errors::ErrorMessage.new(options)
end

    And I don't agree with it. Validator should use record.errors.add.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 25th, 2010 @ 05:43 PM

    Are you suggesting me to change errors.add API to include an additional option?

    I could do that, but that would be a completely different patch. I was trying to avoid changing errors.add API.

    But there is a problem with this approach. For each validator it would be necessary to change the errors.add line to something like:

    # record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => options[:message])
  record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => options[:full_message] || options[:message], :full_message => options.has_key?(:full_message)
# or
  record.errors.add(attr_name, :not_a_number, :value => raw_value, :default => message, :full_message => options.has_key?(:full_message) # with message returning options[:full_message] || options[:message]

    This seems much uglier to me. What do you think?

    But I still think that the current proposed patch would be much more complete and benefic than just adding a :full_message option, don't you think?

  • José Valim

    José Valim February 25th, 2010 @ 08:42 PM

    Why changing each validator is a problem? We are changing it anyway, we just need a method that does it for us:

    def add_error(record, attribute, message, extra_options = {})
  record.errors.add(attribute, message, { :message => message, :full_message => full_message }.merge(extra_options))
end

    Doing it with errors.add allows me to use :full_message in any other place in my code, without requiring knowledge of a class called ErrorMessage, which should be an implementation detail.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 26th, 2010 @ 08:40 PM

    Well, I would document ErrorMessage, anyway.

    Follows a patch for supporting just :full_message, as you have suggested. Is it as you were expecting?

    Please note that some parts could be simplified but the I18n tests would fail because they use mocha to expect a specific Hash would be passed to generate_message... Simplifying the code would require to change a lot of I18n tests...

  • José Valim

    José Valim July 19th, 2010 @ 02:42 PM

    • State changed from “incomplete” to “stale”
    • Importance changed from “” to “Low”

    Someone did exactly what we were discussing above. He used a separate patch to clean up validators and allow any option to be given to validations. He also was motivated enough to simplify the code even if it required to change a lot of I18n tests. :)

    That said, if you are still interested in getting this in, you could rebase your patch. Marking this as stale until we get there.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 14th, 2010 @ 02:48 AM

    I don't know why, but I didn't receive any notification by e-mail about your last update. I just happen to see how this ticket was today and I could see your message... Well, I'll take a look at how Rails base code is at the moment so that I can rebase the patch. Thank you for reporting this.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 24th, 2010 @ 04:16 AM

    Follows an initial patch, without tests and documentation for a first review.

  • José Valim

    José Valim August 24th, 2010 @ 04:22 AM

    I don't think generate_message should care about full_message, you can simply wrap the result of generate_message in errors.add. Also, you should rather implement full_message in the ErrorMessage and check if it is defined instead of checking if you have ErrorMessage (i.e. use duck typing).

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 24th, 2010 @ 11:39 AM

    Second attempt. Remember options is frozen in #add.

  • José Valim

    José Valim August 28th, 2010 @ 02:38 PM

    • Milestone set to “3.1”
    • State changed from “stale” to “open”

    Great! We are almost there! Just a few notes:

    1) I should be able to specify both :message and :full_message. So there is no need to make full_message overwrite message in the hash. Doing this change, you need to make sure :full_message won't be passed as option to the underlying I18n.t in generate_message.

    2) At first I was thinking that we could move generate_message to the ErrorMessage object but I believe it will be better if we don't. That said, there is no need to pass @base and @attributes to the ErrorMessage object. This should be enough:

    ErrorMessage.new(message, options[:full_message])

class ErrorMessage < String
  attr_reader :full_message

  def initialize(message, full_message)
    super(message)
    @full_message = full_message
  end
end

    3) And one final note, I know your patch is a initial version, but be sure to use full names like "full_messages" as variable names instead of "fm". :)

    Thanks and good work!

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 03:50 PM

    Please, take a look if this patch meets your expectative.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 04:07 PM

    Or maybe this is a better approach.

  • José Valim

    José Valim August 28th, 2010 @ 08:17 PM

    Why is the logic used just when message is a symbol? Why we cannot pass "full_message" when message is a proc?

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 08:25 PM

    The patch aims including :full_message to validators. Could you tell me of any validator that passes a non-symbol value to the message parameter?

    Maybe a custom validator? I can make that change, but I changed the logic inside the symbols because I was thinking only in the shipped validators...

    I'll make the change and post a new patch here.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 08:29 PM

    Here it is.

  • José Valim

    José Valim August 28th, 2010 @ 08:29 PM

    Plugins that have date/time validators use extensively the proc support for message because they are based on Time.now or Date.current. Maybe even full_message should support a proc as option, but better to add it as functionality just when someone needs it.

  • José Valim

    José Valim August 28th, 2010 @ 08:29 PM

    Plugins that have date/time validators use extensively the proc support for message because they are based on Time.now or Date.current. Maybe even full_message should support a proc as option, but better to add it as functionality just when someone needs it.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 08:34 PM

    I can do that, that is trivial, just a minute...

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 08:37 PM

    Here it is

  • José Valim

    José Valim August 28th, 2010 @ 08:40 PM

    The :message should not be overwritten by the :full_message, they are different concerns. I should be able to specify both:

    validates_presence_of :name, :message => "cannot be blank", :full_message => "Name cannot be left blank"

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 09:19 PM

    I misunderstood what you have told me before.

    Please, take a look if that is what you were talking about. (I also fixed some bugs...)

  • Ruy Asan

    Ruy Asan August 28th, 2010 @ 09:21 PM

    How are they separate concerns? When would both be used?

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 09:23 PM

    Or maybe you prefer the '&&=' syntax, as in:

    full_message &&= generate_message(attribute, message, options.merge(:message => full_message))

    and

    full_message &&= message

    and

    message &&= ErrorMessage.new(message, full_message) if full_message

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 09:26 PM

    Hi, Ruy (almost Ruby, huh?)

    Valim and I have been changing some messages by e-mail. He thinks that maybe some one would like to have messages appearing twice in a form as in:

    Errors:

    You should specify your full name

    Name field here (error message here, for instance: should not be blank)

    That is what I've understood.

  • José Valim

    José Valim August 28th, 2010 @ 09:45 PM

    I'm not following one thing. Why full_message needs to pass through generate_message()? generate_message() does a lookup under errors.messages which is unrelated to full_messages. They are completely different concerns, they should not be mixed.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 10:19 PM

    The line following line is unnecessary for what you have in mind:

    full_message = generate_message(attribute, message, options.merge(:message => full_message)) if full_message

    The idea is the possibility to write something like

    validates_uniqueness_of :name, :full_message => :name_not_unique

    en.yml:
    en:
    name_not_unique: You should specify an unique name.

    Is this useful? I know that it is possible to override the format in I18n yaml files, but maybe this approach would be useful and consistent with how :message currently works...

    Here is another patch without the line if you prefer.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 10:24 PM

    Actually, options.except(:full_message, *CALLBACKS_OPTIONS) reads better, as in the new attachment.

  • José Valim

    José Valim August 28th, 2010 @ 10:24 PM

    If we are allowing such behavior, the lookup cannot be in the same place as generate_message (which is error.messages). It needs its own namespace like (error.full_messages) (probably a configuration option you can give to generate_message).

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 10:26 PM

    A new patch, just updating ErrorMessage outdated documentation.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 10:36 PM

    Please, take a look if this is what you're talking about.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 10:37 PM

    Sorry, ignore the last message, I'll write another one.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 10:52 PM

    This patch seems more correct to me...

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 28th, 2010 @ 11:03 PM

    I was thinking a bit more about this... Shouldn't it be possible to specify a validation like:

    validates_uniqueness_of :email, :message => lambda{some_method_here}

    With the current API this would not be possible... With the patch, this would be possible for :full_message, but not for :message, as it is already the behavior...

    Should we support this style for normal validations?

    Anyway, is there any use case where passing a Proc makes any sense? I mean, no args are currently being passed to proc.call. The only reason I could see at the moment is to postpone message generation...

    Please, could someone clarify this situation with any real use case?

  • José Valim

    José Valim August 28th, 2010 @ 11:17 PM

    "Shouldn't it be possible to specify a validation like:"

    If we really needed it, someone would already have provided a patch with it as a feature. :)

    Your patch is almost there, the only change I would do is to change generate_message to receive a key as argument:

    generate_message(attributes, message, options, type)

    This way you can skip checking for :full_message inside generate_message and do everything based on the type given.

    Another thing is that, once you write tests, you will see your patch is broken because generate_message is acting destructively in the hash. So the first time it is called, it will remove :full_message and :message from the options hash, so the second call won't work!

  • Ruy Asan

    Ruy Asan August 29th, 2010 @ 12:22 AM

    I think it would be unexpected and inconsistent behavior if :message accepted lambdas but :full_message did not.

  • José Valim

    José Valim August 29th, 2010 @ 12:24 AM

    The feature I was talking about is the ability to call an instance method from the lambda.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas August 29th, 2010 @ 02:47 AM

    I've played a bit with proc params. Please, take a look and let me know what do you think...

  • José Valim

    José Valim September 2nd, 2010 @ 07:52 AM

    Looks good. Just note there is no need to check the arity of the proc. Simple call it with all args (and it is backward compatible).

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 2nd, 2010 @ 12:31 PM

    I don't think it is a good idea. Follow some examples:

    Ruby 1.8.7:

    lambda{}.arity == -1; lambda{}.call(*[1,2,3]) # OK, no error

    Ruby 1.9.2:

    lambda{}.arity == 0; lambda{}.call(*[1,2,3]) # ArgumentError: wrong number of arguments (3 for 0)

    Both 1.8 and 1.9:
    lambda{|a|}.arity == 1; lambda{}.call(*[1,2,3]) # ArgumentError: wrong number of arguments (3 for 1)

    I don't understand why you are saying it is backward compatible... Could you please, clarify?

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 2nd, 2010 @ 12:32 PM

    The last example should be read:
    lambda{|a|}.arity == 1; lambda{|a|}.call(*[1,2,3]) # ArgumentError: wrong number of arguments (3 for 1)

  • José Valim

    José Valim September 2nd, 2010 @ 12:41 PM

    Hrm, sorry! I forgot that lambda in Ruby 1.8.7 does not enforce the arity properly. Not sure I like the current patch though, can we simply not pass any parameter to the proc, as we do today?

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 2nd, 2010 @ 12:50 PM

    This is what already happens on full_messages-v10.patch, but it is only useful for things like Time.now...

    The last patch allows the use of the record attribute values on error messages, making procs more useful, while still being backward compatible.

  • José Valim

    José Valim September 2nd, 2010 @ 12:52 PM

    Yes, but it also adds a lot of complexity to the code. Remember that you can add validations from the instance level, where there is no need for a proc and you have all the customization and access to attributes, options and values you need.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 2nd, 2010 @ 12:59 PM

    Ok, that was why I asked about you opinion on the last patch. Then, what do you think about v10 patch?

  • José Valim

    José Valim September 2nd, 2010 @ 01:02 PM

    My comments for patch v10 are above. I will copy them for convenience:

    Your patch is almost there, the only change I would do is to change generate_message to receive a key as argument:

    generate_message(attributes, message, options, type)

    This way you can skip checking for :full_message inside generate_message and do everything based on the type given.

    Another thing is that, once you write tests, you will see your patch is broken because generate_message is acting destructively in the hash. So the first time it is called, it will remove :full_message and :message from the options hash, so the second call won't work (because it won't have any)! But this should be fixed by generating the message as I just mentioned above.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 2nd, 2010 @ 01:09 PM

    Ok, sorry. Please, verify if I understood the approach you are suggested in generate_message for v11.

  • José Valim

    José Valim September 2nd, 2010 @ 01:25 PM

    I worked a bit in the generate_message in this diff as an example:

    http://gist.github.com/562212

    However, note that some places I cannot use #{key} in the lookup meaning that we are going to look into the same place for both full_messages and messages, which is going to cause a bug.

    One option is to, instead of looking up in "errors.full_messages" for full_messages, we could look at "full_errors.messages". It solves the issue, but seems weird. What do you think?

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 2nd, 2010 @ 02:04 PM

    I commented on the gist, but thinking more about the problem, maybe we are overcomplicating the problem...

    If we don't support the feature of passing both :message and :full_message (giving preference to :full_message in this case), the patch would be much smaller and less confuse...

    Do you really think passing both :message and :full_message is that useful? Or maybe could we support this feature in a later patch?

  • José Valim

    José Valim September 2nd, 2010 @ 05:11 PM

    I'm a 100% sure that if we don't allow both to be passed, it is going
    to cause confusion and subsequent reports in LH. I'm saying that
    because this is what happened in 2.3 branch. :)

    On Sep 2, 2010, at 15:05, "Lighthouse" <no-reply@lighthouseapp.com>
    wrote:

  • windock

    windock September 2nd, 2010 @ 06:07 PM

    For sure both options should be allowed to pass. Think of this scenario. You have 2 kind of forms: first is with inline errors, second one is usual. For inline ones, you'd want to show just "this can't be blank, dude", but for regular one - "Dude, please, fill in username". Also, you'd want to show full errors passed through flash (say, you want to show errors on different page) Currently it's not possible to do this nicely. At all. And this is kind of situation I have with my project, so it is very important feature.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 2nd, 2010 @ 06:37 PM

    Ok, I'll give it more thoughts tonight when I get home. I need to think a bit about how to achieve that without using full_errors or think in a better name for it...

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 3rd, 2010 @ 12:47 AM

    Valim, I was thinking more about I18n keys, I think I would prefer something like:

    don't change signature, avoiding rewriting other tests with no advantages with this approach...

    def generate_message(attribute, type = :invalid, options = {})
    type = options.delete(key) if options[key].is_a?(Symbol) type = "full#{type}" if options[:full_message] ... end

    Then, for the default backend, we could write something like:

    en:
    activerecord:

    errors:
  models:
    user:
      attributes:
        name:
          blank: should be specified
          _full_blank: You must specify your name

    I think it is much easier to maintain than:

    en:
    activerecord:

    errors:
  models:
    user:
      attributes:
        name:
          blank: should be specified
full_errors:
  models:
    user:
      attributes:
        name:
          blank: You must specify your name

    What do you think?

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 3rd, 2010 @ 12:49 AM

    Sorry for not using Preview... The h1 text was meant to be a Ruby comment... :)

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 3rd, 2010 @ 12:50 AM

    Trying again (Please remove last comments):

    # don't change signature, avoiding rewriting other tests with no advantages with this approach...
def generate_message(attribute, type = :invalid, options = {})
  type = options.delete(key) if options[key].is_a?(Symbol)
  type = "_full_#{type}" if options[:full_message]
  ...
end

Then, for the default backend, we could write something like:

en:
  activerecord:
    errors:
      models:
        user:
          attributes:
            name:
              blank: should be specified
              _full_blank: You must specify your name

I think it is much easier to maintain than:

en:
  activerecord:
    errors:
      models:
        user:
          attributes:
            name:
              blank: should be specified
    full_errors:
      models:
        user:
          attributes:
            name:
              blank: You must specify your name

    What do you think?

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 4th, 2010 @ 12:44 AM

    Actually (I was re-reading my post), I meant 'options.delete(:message)' instead of 'options.delete(key)'...

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 4th, 2010 @ 02:19 PM

    I've just found one more issue with generate_message...

    While moving an application to Rails3, I replaced the passive_record plugin with ActiveModel. I'm facing a problems with current generate_message approach.

    The class is under a module namespace and klass.model_name.underscore yields 'module_name/class_name'. I think 'module_name.class_name' would suit better for generate_message (something like "klass.model_name.underscore.gsub('/', '.')")...

    What do you think?

  • José Valim

    José Valim September 5th, 2010 @ 09:01 AM

    Yes, you are right, this is a bug. Could you please provide a patch with test in another ticket? :D

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas September 7th, 2010 @ 02:44 PM

    I've created ticket #5572... And, please, tell me what you think about my suggestion for the current ticket...

  • Ryan Bigg

    Ryan Bigg October 9th, 2010 @ 10:12 PM

    • Tag cleared.

    Automatic cleanup of spam.

  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer October 10th, 2010 @ 01:02 PM

    • Title changed from “[PATCH] Add :full_message option to validations” to “Add :full_message option to validations”
    • Tag set to “patch, validations”

    Using the "patch" tag instead of prefixing the ticket title with "[PATCH]" to make sure patched tickets end up in the open patches bin. :)

  • spovich

    spovich January 14th, 2011 @ 07:02 PM

    +1, would love to see this change. I like Rodrigo's suggestion of providing the full message right next to the regular message. This keeps similar string next to each other which is better semantically.

  • Lailson Bandeira

    Lailson Bandeira January 14th, 2011 @ 07:46 PM

    +1, this is a very useful feature, especially for some languages (like portuguese =D).

  • Dmitry Polushkin

    Dmitry Polushkin January 24th, 2011 @ 05:50 PM

    • Tag changed from “patch, validations” to “full_message, full_messages, patch, validations”

    Please update state of this feature.

  • Dmitry Polushkin

    Dmitry Polushkin February 4th, 2011 @ 03:25 PM

    • Tag changed from “full_message, full_messages, patch, validations” to “activemodel, errors, full_message, full_messages, patch, validations”

    Nearly the same functionality can be found here: https://github.com/svenfuchs/i18n-message/

    1) Store symbols instead of string messages;
    2) Generate messages for full/short/etc. using symbols by requirement (possible chached);
    3) OrderedHash should be structured in hierarchy, not in flat error array, eg:

    {
  :name => [:blank],
  :email => [:blank, :invalid]
}

    So it will be possible to translate it on the remote activerecord server (for example). Right now error messages almost useless in ActiveResource, because it's impossible to detect which errors on which attributes happend.

    If everything is clear and just implementation need, I can try to do it.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 4th, 2011 @ 04:18 PM

    Hi Dmitry,

    I think the i18n-message plugin approach is much more complex than what I've proposed. I prefer

    validates_presence_of :some_attribute, :full_message => 'My custom full message'

    over

    validates_presence_of :foo, :message => { :full => 'My custom full message.' }, :format => { :full => '{{message}}' }

  • Dmitry Polushkin

    Dmitry Polushkin February 4th, 2011 @ 05:05 PM

    Rodrigo,

    How to translate full_message in this case? I mean how it will be connected to the different locales?

    In approach that I've listed above, it's possible to have a message with different formats. Also it fixes another bugs (for example in ActiveResource::Error class for i18n strings). And code will be more flexible, plus without hacks.

  • Dmitry Polushkin

    Dmitry Polushkin February 4th, 2011 @ 05:09 PM

    Some off-topic (but anyway still related) about the ActiveResource. Look to this code to understand why it's bad:

    def from_array(messages, save_cache = false)
  clear unless save_cache
  humanized_attributes = Hash[@base.attributes.keys.map { |attr_name| [attr_name.humanize, attr_name] }]
  messages.each do |message|
    attr_message = humanized_attributes.keys.detect do |attr_name|
      if message[0, attr_name.size + 1] == "#{attr_name} "
        add humanized_attributes[attr_name], message[(attr_name.size + 1)..-1]
      end
    end

    self[:base] << message if attr_message.nil?
  end
end

    I think, if ActiveModel::Errors have an OrderedHash with the sturcture that I've written in the three posts before, then it will be possible to detect correct error columns in the ActiveResource.

    Sorry for a small off-topic, but I think it should be discussed here also.

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 4th, 2011 @ 06:18 PM

    Hi Dmitry, I was not talking about implementation but about the API.

    I don't know about the i18n-message implementation. And it really doesn't matter to me. I'm just saying that I would be happy to use i18n-message if its API allowed me to use 'validates... :full_message => "My custom message"'. It's much simpler for the common case.

    About i18n, I think the following API would also be interesting:

    validates_presence_of :some_attribute, :full_message => :"some.i18n.symbol.here"

    This could be translated by the gem to

    validates_presence_of :some_attribute, :message => {:full => :"some.i18n.symbol.here"}, format => {:full => '{{message}}'}
  • Dmitry Polushkin

    Dmitry Polushkin February 4th, 2011 @ 08:21 PM

    Rodrigo,

    I'm talking also about API :) (sorry_for_my_bad_english unless something_could_be_understood).

    Actually good idea to use :message => {:full => 'message'}, but for the i18n I'd prefer splitted version, that you quoted above:

    en:
  activerecord:
    errors:
      models:
        user:
          attributes:
            name:
              blank: should be specified
    full_errors:
      models:
        user:
          attributes:
            name:
              blank: You must specify your name

    So it will be generated automatically based on those locale strings. So no:

    :message => {:full => :"some.i18n.symbol.here"}
  • Dmitry Polushkin

    Dmitry Polushkin February 4th, 2011 @ 08:25 PM

    But only one thing, if you are generating short (standard) message, then it will be taken from short, if you are generating it using full_messages, then it will be taken from full, if not found then formatted using errors.format.full, like %{attribute} %{message} (or whatever).

  • Rodrigo Rosenfeld Rosas

    Rodrigo Rosenfeld Rosas February 5th, 2011 @ 01:47 AM

    Dmitry, I didn't understand your message very well.

    I didn't propose using ":message => {:full => 'message'}". I think the syntax is too big for the common case of specifying a full message for some validation. I just said that I think the proposed API could be internally translated to the former style in the mentioned plugin.

    The proposed API should be useful both for i18n applications and one-language only applications without adding much overhead for the simple task of defining a custom full message for some validation (like creating a i18n yaml even if your application doesn't require i18n).

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages