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

Allow to override the full_message error format #32956

Merged
merged 2 commits into from Jun 11, 2018

Conversation

Projects
None yet
4 participants
@Larochelle
Contributor

Larochelle commented May 22, 2018

Summary

The goal of this PR is to make it easier for an app to transition from a #{attribute} #{message} to a #{message}, full_message error format.

full_message formats error messages with a #{attribute} #{message} format which generates error messages as “name cannot be nil”.

It is possible to override the format with :"errors.format" but only language wide. https://github.com/rails/rails/blob/master/activemodel/lib/active_model/errors.rb#L371 allows a language to define a custom format, but changing it forces to extract all messages, including the implicit ones as :blank

The #{attribute} #{message} format prevents languages to move the attribute name to somewhere else within the message, for example, in some cases it can be preferred to have error messages as “The person's name cannot be blank”, so changing the format to #{message} and including the attribute name in the message itself can be preferable.

To make such transition easier, this PR allows to override errors.format at the attribute or model level, so an app can use either:

en:
  errors:
    format: '%{message}'
en:
  activemodel:
    errors:
      models:
        person:
          format: '%{message}'
en:
  activemodel:
    errors:
      models:
        person:
          attributes:
            name:
              format: '%{message}'

How

This is based on active_model/translation.rb#human_attribute_name
https://github.com/rails/rails/blob/master/activemodel/lib/active_model/translation.rb#L44

And active_model/errors.rb#generate_message
https://github.com/rails/rails/blob/master/activemodel/lib/active_model/errors.rb#L401

  • The @base.class.respond_to?(:i18n_scope) is needed for cases where ActiveModel::Errors is used without including the Translation module

Benchmark

require "active_model"

module ActiveModel
  class Errors
    def full_message_i18n(attribute, message)
      return message if attribute == :base

      if @base.class.respond_to?(:i18n_scope)
        parts = attribute.to_s.split(".")
        attribute_name = parts.pop
        namespace = parts.join("/") unless parts.empty?
        attributes_scope = "#{@base.class.i18n_scope}.errors.models"

        if namespace
          defaults = @base.class.lookup_ancestors.map do |klass|
            [
              :"#{attributes_scope}.#{klass.model_name.i18n_key}/#{namespace}.attributes.#{attribute_name}.format",
              :"#{attributes_scope}.#{klass.model_name.i18n_key}/#{namespace}.format",
            ]
          end
        else
          defaults = @base.class.lookup_ancestors.map do |klass|
            [
              :"#{attributes_scope}.#{klass.model_name.i18n_key}.attributes.#{attribute_name}.format",
              :"#{attributes_scope}.#{klass.model_name.i18n_key}.format",
            ]
          end
        end

        defaults.flatten!
      else
        defaults = []
      end

      defaults << :"errors.format"
      defaults << "%{attribute} %{message}"

      attr_name = attribute.to_s.tr(".", "_").humanize
      attr_name = @base.class.human_attribute_name(attribute, default: attr_name)
      I18n.t(defaults.shift,
        default:  defaults,
        attribute: attr_name,
        message:   message)
    end
  end
end

class Person
  include ActiveModel::Validations
  extend  ActiveModel::Translation

  attr_accessor :title, :karma, :salary, :gender

  def condition_is_true
    true
  end

  def condition_is_false
    false
  end
end

person = Person.new

# Enumerate some representative scenarios here.
#
# It is very easy to make an optimization that improves performance for a
# specific scenario you care about but regresses on other common cases.
# Therefore, you should test your change against a list of representative
# scenarios. Ideally, they should be based on real-world scenarios extracted
# from production applications.
SCENARIOS = {
  "Deeply nested model attributes" => {
    attribute: :'contacts/addresses.street',
    translations: { errors: { format: "%{attribute} %{message}" } }
  },
  "Attribute with base format" => {
    attribute: :name,
    translations: { errors: { format: "%{attribute} %{message}" } }
  },
  "Attribute with attribute format" => {
    attribute: :name,
    translations: { activemodel: { errors: { models: { person: { attributes: { name: { format: "%{message}" } } } } } } }
  }
}

SCENARIOS.each_pair do |name, value|
  attribute = value[:attribute]
  translations = value[:translations]

  I18n.load_path.clear
  I18n.backend = I18n::Backend::Simple.new
  I18n.backend.store_translations("en", translations)

  puts
  puts " #{name} ".center(80, "=")
  puts

  Benchmark.ips do |x|
    x.report("full_message_i18n") { person.errors.full_message_i18n(attribute, "cannot be blank") }
    x.report("full_message") { person.errors.full_message(attribute, "cannot be blank") }
    x.compare!
  end
end

Results

The result with the variance between runs

======================== Deeply nested model attributes ========================

Warming up --------------------------------------
   full_message_i18n     1.011k i/100ms
        full_message     1.423k i/100ms
Calculating -------------------------------------
   full_message_i18n     10.145k (± 3.9%) i/s -     51.561k in   5.090447s
        full_message     14.456k (± 3.9%) i/s -     72.573k in   5.028127s

Comparison:
        full_message:    14456.1 i/s
   full_message_i18n:    10144.8 i/s - 1.38x - 1.44x  slower


========================== Attribute with base format ==========================

Warming up --------------------------------------
   full_message_i18n     1.203k i/100ms
        full_message     1.834k i/100ms
Calculating -------------------------------------
   full_message_i18n     12.047k (± 4.0%) i/s -     60.150k in   5.000757s
        full_message     17.978k (± 1.9%) i/s -     89.866k in   5.000616s

Comparison:
        full_message:    17977.9 i/s
   full_message_i18n:    12047.3 i/s - 1.49x - 1.65x  slower


======================= Attribute with attribute format ========================

Warming up --------------------------------------
   full_message_i18n     1.553k i/100ms
        full_message     1.772k i/100ms
Calculating -------------------------------------
   full_message_i18n     16.465k (± 3.9%) i/s -     82.309k in   5.006884s
        full_message     17.041k (± 2.7%) i/s -     86.828k in   5.099021s

Comparison:
        full_message:    17040.9 i/s
   full_message_i18n:    16464.7 i/s - same-ish: difference falls within error to  1.20x  slower
@sikachu

I think this is a good patch. I personally found myself having to find way to workaround this a few times. It'd be nice to be able to do this in i18n config.

wdyt @rafaelfranca @matthewd ?

@sikachu sikachu added the activemodel label May 25, 2018

activemodel/lib/active_model/railtie.rb Outdated
@@ -7,8 +7,15 @@ module ActiveModel
class Railtie < Rails::Railtie # :nodoc:
config.eager_load_namespaces << ActiveModel
config.active_model = ActiveSupport::OrderedOptions.new
config.app_generators.orm :active_model

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 1, 2018

Member

Weird. Why did you need to add this? Active Model is not a ORM.

This comment has been minimized.

@Larochelle

Larochelle Jun 5, 2018

Contributor

Removed.
Looking the active_record and http://api.rubyonrails.org/classes/Rails/Railtie.html as examples, I though it was needed.

@@ -12,13 +12,17 @@ def setup
I18n.load_path.clear
I18n.backend = I18n::Backend::Simple.new
I18n.backend.store_translations("en", errors: { messages: { custom: nil } })
@original_i18n_full_message = ActiveModel::Errors.i18n_full_message
ActiveModel::Errors.i18n_full_message = true

This comment has been minimized.

@rafaelfranca

This comment has been minimized.

@Larochelle

Larochelle Jun 5, 2018

Contributor

Added

@rafaelfranca rafaelfranca merged commit d3f659e into rails:master Jun 11, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
class << self
attr_accessor :i18n_full_message # :nodoc:
end
self.i18n_full_message = false

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jun 11, 2018

Contributor

Is it added intentionally?

It seems extra since we set default value of i18n_full_message in activemodel/lib/active_model/railtie.rb.

diff --git a/activemodel/lib/active_model/errors.rb b/activemodel/lib/active_model/errors.rb
index 56404a036c..7400bb5a6c 100644
--- a/activemodel/lib/active_model/errors.rb
+++ b/activemodel/lib/active_model/errors.rb
@@ -65,7 +65,6 @@ class Errors
     class << self
       attr_accessor :i18n_full_message # :nodoc:
     end
-    self.i18n_full_message = false

     attr_reader :messages, :details

diff --git a/activemodel/lib/active_model/railtie.rb b/activemodel/lib/active_model/railtie.rb
index 0ed70bd473..ed70d65c8e 100644
--- a/activemodel/lib/active_model/railtie.rb
+++ b/activemodel/lib/active_model/railtie.rb
@@ -8,13 +8,14 @@ class Railtie < Rails::Railtie # :nodoc:
     config.eager_load_namespaces << ActiveModel

     config.active_model = ActiveSupport::OrderedOptions.new
+    config.active_model.i18n_full_message = false

     initializer "active_model.secure_password" do
       ActiveModel::SecurePassword.min_cost = Rails.env.test?
     end

     initializer "active_model.i18n_full_message" do
-      ActiveModel::Errors.i18n_full_message = config.active_model.delete(:i18n_full_message) || false
+      ActiveModel::Errors.i18n_full_message = config.active_model.delete(:i18n_full_message)
     end
   end
 end

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 11, 2018

Member

Yes, it is intentional. We want a default value if the railtie is not loaded, for example if you use Active Model without Rails.

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Jun 11, 2018

Contributor

It makes sense then. Thanks for explanation.
What do you think about making ActiveModel::Errors.i18n_full_message as public API in order to allow users use this new feature if use Active Model without Rails?

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Jun 11, 2018

Fix active_model/errors docs [ci skip]
- Fix indentation.
- Add a missing dot to the end of the sentence.

Related to rails#32956

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Jun 11, 2018

Add changelog for rails#32956 [ci skip]
Add mention about default value of `config.active_model.i18n_full_message`.

kamipo added a commit that referenced this pull request Jun 11, 2018

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