Deprecate `ActiveModel::Errors` `get`, `set` and `[]=` methods. #18634

Merged
merged 1 commit into from Feb 18, 2015

Conversation

Projects
None yet
4 participants
@morgoth
Member

morgoth commented Jan 21, 2015

They have inconsistent behaviour currently.

As discussed in #18631

Do you think we can go further and deprecate also add_on_blank and add_on_empty?
Not sure what is the usecase for them and it's easy enough to write it on your own when needed.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jan 22, 2015

Contributor

In complex service objects, we use add_on_empty quite a lot, as there are several pathways that may lead to the same error key being set, and I don't want the same error populated multiple times and dumped as redundant text on user validation error display. Sure I could write my own check, but why not keep this simple helper?

Contributor

egilburg commented Jan 22, 2015

In complex service objects, we use add_on_empty quite a lot, as there are several pathways that may lead to the same error key being set, and I don't want the same error populated multiple times and dumped as redundant text on user validation error display. Sure I could write my own check, but why not keep this simple helper?

activemodel/lib/active_model/errors.rb
+ ActiveModel::Errors#set is deprecated and will be removed in Rails 5.1
+
+ To achieve the same use
+ delete(:#{key}) followed by

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 22, 2015

Member

Does it need to call delete?

@rafaelfranca

rafaelfranca Jan 22, 2015

Member

Does it need to call delete?

This comment has been minimized.

@morgoth

morgoth Jan 22, 2015

Member

It doesn't need to - already fixed.

@morgoth

morgoth Jan 22, 2015

Member

It doesn't need to - already fixed.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 22, 2015

Member

@egilburg I can see that add_on_empty is a "light" version of PresenceValidator, but add_on_blank is exactly the same as PresenceValidator, so maybe at least this one can be deprecated?

Member

morgoth commented Jan 22, 2015

@egilburg I can see that add_on_empty is a "light" version of PresenceValidator, but add_on_blank is exactly the same as PresenceValidator, so maybe at least this one can be deprecated?

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jan 22, 2015

Contributor

Huh? I thought add on empty means add error if model doesn't have already error with that key.

Eugene

On Jan 22, 2015, at 11:23 AM, Wojciech Wnętrzak notifications@github.com wrote:

@egilburg I can see that add_on_empty is a "light" version of PresenceValidator, but add_on_blank is exactly the same, so maybe at least this one can be deprecated?


Reply to this email directly or view it on GitHub.

Contributor

egilburg commented Jan 22, 2015

Huh? I thought add on empty means add error if model doesn't have already error with that key.

Eugene

On Jan 22, 2015, at 11:23 AM, Wojciech Wnętrzak notifications@github.com wrote:

@egilburg I can see that add_on_empty is a "light" version of PresenceValidator, but add_on_blank is exactly the same, so maybe at least this one can be deprecated?


Reply to this email directly or view it on GitHub.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 22, 2015

Member

@egilburg Nope, it adds :empty error when given value is empty.

u = User.new
u.errors.add(:email, :invalid)
u.errors.add_on_empty(:email)
# => [:email]
u.errors.full_messages
# => ["Email is invalid", "Email can't be empty"]

It also adds this error when value is nil, so it behaves almost exactly as add_on_blank

Member

morgoth commented Jan 22, 2015

@egilburg Nope, it adds :empty error when given value is empty.

u = User.new
u.errors.add(:email, :invalid)
u.errors.add_on_empty(:email)
# => [:email]
u.errors.full_messages
# => ["Email is invalid", "Email can't be empty"]

It also adds this error when value is nil, so it behaves almost exactly as add_on_blank

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 25, 2015

Member

@egilburg Did I understand you correctly, that you misused add_on_empty method? If yes, would you agree now on deprecating it?

Member

morgoth commented Jan 25, 2015

@egilburg Did I understand you correctly, that you misused add_on_empty method? If yes, would you agree now on deprecating it?

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jan 25, 2015

Contributor

@morgoth yes it doesn't seem to be what I thought it is.

Contributor

egilburg commented Jan 25, 2015

@morgoth yes it doesn't seem to be what I thought it is.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 27, 2015

Member

@rafaelfranca this PR is good to go. WDYT about deprecating add_on_empty and add_on_blank as stated above?

Member

morgoth commented Jan 27, 2015

@rafaelfranca this PR is good to go. WDYT about deprecating add_on_empty and add_on_blank as stated above?

Deprecate `ActiveModel::Errors` `get`, `set` and `[]=` methods.
They have inconsistent behaviour currently.
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 18, 2015

Member

Yeah, let deprecated those methods too.

Member

rafaelfranca commented Feb 18, 2015

Yeah, let deprecated those methods too.

@rafaelfranca rafaelfranca merged commit 6ec8ba1 into rails:master Feb 18, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Feb 18, 2015

Merge pull request #18634 from morgoth/deprecate-some-errors-methods
Deprecate `ActiveModel::Errors` `get`, `set` and `[]=` methods.

@morgoth morgoth deleted the morgoth:deprecate-some-errors-methods branch Feb 18, 2015

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Feb 20, 2015

Member

For people coming here from This week in Rails, see f55bfe7.

Summary:

  • model.errors[:name]
  • model.errors.add(:name, ...)
  • 👎 model.errors.get(:name)
  • 👎 model.errors.set(:name, ...)
  • 👎 model.errors[:name] = ...
Member

chancancode commented Feb 20, 2015

For people coming here from This week in Rails, see f55bfe7.

Summary:

  • model.errors[:name]
  • model.errors.add(:name, ...)
  • 👎 model.errors.get(:name)
  • 👎 model.errors.set(:name, ...)
  • 👎 model.errors[:name] = ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment