Activemodal::Errors added methods error_message_format and i18n_priority_format #14260

Open
wants to merge 4 commits into
from

Projects

None yet

10 participants

@TemaMix
TemaMix commented Mar 3, 2014

error_message_format - returns an I18n key of an error message taking into consideration the format of an error message. If none found the default value is being returned (errors.format).

Formats are first looked up in "activerecord.errors.models.MODEL.attributes.ATTRIBUTE.format", if it's not there, it's looked up in "activerecord.errors.models.MODEL.format" and if that is not there also, it returns default format (e.g. "errors.format").

i18n_priority_format - returns array that defined format priority key for i18n scope an error message. Last array element have highest priority.
person.errors.i18n_priority_format # => [:"errors.format"]
person.errors.i18n_priority_format(:name)
# => [:"errors.format",
# :"activemodel.errors.models.person.format",
# :"activemodel.errors.models.person.attributes.name.format"]

@dmathieu dmathieu commented on an outdated diff Mar 3, 2014
@@ -6,6 +6,8 @@ debug.log
/.bundle
/.ruby-version
/Gemfile.lock
+/.idea
+.DS_Store
@dmathieu
dmathieu Mar 3, 2014

💥 You shouldn't add these.

@dmathieu dmathieu commented on the diff Mar 3, 2014
activemodel/lib/active_model/errors.rb
+ end
+
+ # Returns array that defined format priority key for i18n scope an error message.
+ # Last array element have highest priority.
+ #
+ # person.errors.i18n_priority_format # => [:"errors.format"]
+ # person.errors.i18n_priority_format(:name)
+ # # => [:"errors.format",
+ # :"activemodel.errors.models.person.format",
+ # :"activemodel.errors.models.person.attributes.name.format"]
+ def i18n_priority_format(attribute=nil)
+ return [:"errors.format"] unless attribute and @base.class.respond_to?(:i18n_scope)
+ [:"errors.format",
+ :"#{@base.class.i18n_scope}.errors.models.#{@base.class.model_name.i18n_key}.format",
+ :"#{@base.class.i18n_scope}.errors.models.#{@base.class.model_name.i18n_key}.attributes.#{attribute}.format"]
+ end
@dmathieu
dmathieu Mar 3, 2014

Any reason for making these two methods public? That doesn't seem necessary.

@TemaMix
TemaMix Mar 3, 2014

@dmathieu You are right, no reason. I will bear them in private.

@carlosantoniodasilva

I'm not sure about this, if I'm not wrong something like that has been already proposed. There's a performance consideration to take into account that this adds tons of extra I18n lookups for a single attribute error.

Can you give more context on what's your use case for this? Thanks.

@carlosantoniodasilva

#7369 is another take on this one.

@TemaMix
TemaMix commented Mar 3, 2014

@carlosantoniodasilva

Example use case:

errors:
  format: '%{attribute}  %{message}'
activerecord:
 errors:
   attributes:
     name:
       blank: can't be blank
     phone:
        blank: can't be blank

User received error message: Name can't be blank. Phone can't be blank.
Name can't be blank - the customer is arranging this phrase, but the second phrase "phone can't be blank", I would like to change to - "enter your phone"

Current version "full_message" method not available this case.

errors:
  format: '%{attribute}  %{message}'
activerecord:
 errors:
   attributes:
     name:
       blank: can't be blank
     phone:
        blank: enter your phone
        format: '%{message}'

User received error message: Name can't be blank. Enter your phone.

@TemaMix
TemaMix commented Mar 3, 2014

@carlosantoniodasilva
Benchmark test:

DB without format (avr) use priority format (avr)
mysql 0.000413 0.000381
mysql2 0.000328 0.000351
sqlite3 0.000270 0.000371
postgresql 0.000407 0.000536
@philipgiuliani

Any update on this? :(

@rafaelfranca
Member

@TemaMix can you provide the code you used to benchmark?

@eprothro

I've been looking at this as well, and agree the concept of being able to modify the full message format with more granularity is needed.

I think there are a few things to discuss here, but let's start with:

Why does this same concept exist in 2.3 and not in 3.0?

rails 2.3.4 with support for modifying full_messages format per attribute:
https://github.com/rails/rails/blob/v2.3.4/activerecord/lib/active_record/validations.rb#L117-L118

rails 3.0.0 with support for only a single full_messages format:
https://github.com/rails/rails/blob/v3.0.0/activemodel/lib/active_model/errors.rb#L245

I'm not exactly sure why this is not in 3.x and later. My hunch is that it wasn't intentional, and due to the timing of 3.x development the change was implemented on 2.x and failed to be ported to 3.x. If someone else knows for sure, that would be a helpful place to start.


To your point @carlosantoniodasilva and @rafaelfranca, the lookup performance consideration seems to be the next question. Per that, here's a benchmark:

https://gist.github.com/eprothro/c6712e1644abd3379d06

It's what you would expect, N lookups takes N times longer. However, we're already doing N lookups in generate_message.


The more I've thought about it, the more I think, if I were being told about this interface fresh, I would expect to have control over full_message just like I do the error message.

As used by generate_message currently:

# en.yml
en:
  activerecord:
    errors:
      models:
        users:
          invalid: "validation error message" # priority 2
          attributes:
            name:
              invalid: "validation error message" # priority 1
      messages:
        invalid: "validation error message" # priority 3

If full_message were similar:

# en.yml
en:
  activerecord:
    errors:
      full_messages:
        users:
          invalid: "Full validation message" # priority 2
          attributes:
            invalid: "Full validation message" # priority 1

Allowing the example of a one-off full message without the prepended attribute:

# en.yml
en:
  activerecord:
    errors:
      full_messages:
        users:
          attributes:
            invalid: "%{message}"


Maybe let's start by discussing this design before getting into the implementation implications?

@sgrif sgrif was assigned by rails-bot Oct 20, 2015
@rrcdr
rrcdr commented Feb 21, 2016

Was this solved?

@dsbonev
dsbonev commented Feb 29, 2016

👍 A lot of questions on StackOverflow related to this one. Let's fix this.

@sockmonk
sockmonk commented May 1, 2016

I'd like to be able to do this so I don't have to use the custom_error_message gem and prepend "^" to every message that the customer doesn't want to start with the attribute name.

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