Simplify and alias ActiveModel::Errors methods where possible

[morgoth]

No description provided.

[sgrif]

This changes the behavior of the method when given a nil value.

[morgoth]

Can you provide an example?
I think that size must return an integer as it's a simple call on hash like: {}.values.size

[sgrif]

{ foo: nil }

[morgoth]

Not sure if I follow:

irb(main):014:0> {foo: nil}.all? { |k, v| v && v.empty? && !v.is_a?(String) }
=> false
irb(main):015:0> {foo: nil}.values.size.zero?
=> false

It returns the same result

[sgrif]

Sorry {foo: []}

[sgrif]

Our test coverage is not 100%. I'd find the commit that introduced it
before assuming it's safe to remove

On Fri, Feb 20, 2015, 2:56 PM Wojciech Wnętrzak notifications@github.com
wrote:

In activemodel/lib/active_model/errors.rb
#19021 (comment):

 # Returns +true+ if no errors are found, +false+ otherwise.
 # If the error message is a string it can be empty.
 #
 #   person.errors.full_messages # => ["name cannot be nil"]
 #   person.errors.empty?        # => false
 def empty?

•  all? { |k, v| v && v.empty? && !v.is_a?(String) }
  

•  size.zero?
  

Actually there is also a flatten call so it still returns the same value
as before.

irb(main):043:0> {foo: []}.all? { |k, v| v && v.empty? && !v.is_a?(String) }
=> true
irb(main):044:0> {foo: []}.values.flatten.size.zero?
=> true

There is a difference when error is a {}

irb(main):053:0> {foo: {}}.all? { |k, v| v && v.empty? && !v.is_a?(String) }
=> true
irb(main):054:0> {foo: {}}.values.flatten.size.zero?
=> false

Previously size and count returned different results for [] case - size
#=> 0, count #=> 1.
There is no test for adding [], {} as error value and no documentation of
such usage and I don't know why someone would want to add such error.
Should we support it?
If yes, what should be the proper output, because now it's totally
inconsistent.

—
Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/19021/files#r25106561.

[morgoth]

I found related commits:
d748cc3
66e747b

All this cases are true now as well.

size and count inconsistency is for quite long but it doesn't look like it was intentional 8828b2c

[morgoth]

I just found similar old PR where @rafaelfranca addresses issue with error objects other than proc, symbol and string as invalid. #11183 (comment)
Do you think we should check error type on add method and raise ArgumentError when it's not one of the three?

[morgoth]

Are there any more doubts about this PR?
There is no breakage of promised API and 5.0 is a good time to make it clean.

[morgoth]

@rafaelfranca can we at least set 5.0.0 milestone to not forget about this PR?