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

Deprecate `ActiveModel::Errors` `add_on_empty` and `add_on_blank` methods #18996

Merged
merged 1 commit into from Feb 19, 2015

Conversation

Projects
None yet
3 participants
@morgoth
Member

morgoth commented Feb 18, 2015

Follow up to #18634 (comment)

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Feb 18, 2015

Member

I'm not sure if we should delete this test https://github.com/rails/rails/pull/18996/files#diff-399b5cddc709ef96edf30de2328934deR56 as it doesn't have any equivalent in regular validations and :empty symbol is used only in deprecated add_on_empty method

Member

morgoth commented Feb 18, 2015

I'm not sure if we should delete this test https://github.com/rails/rails/pull/18996/files#diff-399b5cddc709ef96edf30de2328934deR56 as it doesn't have any equivalent in regular validations and :empty symbol is used only in deprecated add_on_empty method

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 19, 2015

Member

@morgoth let's remove that test when we remove the deprecated methods and keep it around for now.

Member

senny commented Feb 19, 2015

@morgoth let's remove that test when we remove the deprecated methods and keep it around for now.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 19, 2015

Member

Even though we do not provide a direct replacement the deprecation message should give a hint where to look for similar behavior. It should be trivial to figure out what to change or where to look for more details.

Member

senny commented Feb 19, 2015

Even though we do not provide a direct replacement the deprecation message should give a hint where to look for similar behavior. It should be trivial to figure out what to change or where to look for more details.

@senny senny added the activemodel label Feb 19, 2015

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Feb 19, 2015

Member

@senny I added tip to use PresenceValidator. In add_on_empty it's almost true ;-), as I'm not sure if we should describing implementation details here.

Member

morgoth commented Feb 19, 2015

@senny I added tip to use PresenceValidator. In add_on_empty it's almost true ;-), as I'm not sure if we should describing implementation details here.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 19, 2015

Member

The thing is that PresenceValidator is # :nodoc:. Don't think we should encourage it's use.

Member

senny commented Feb 19, 2015

The thing is that PresenceValidator is # :nodoc:. Don't think we should encourage it's use.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Feb 19, 2015

Member

What about "To achieve the same use validates_presence_of with :empty message"?

Member

morgoth commented Feb 19, 2015

What about "To achieve the same use validates_presence_of with :empty message"?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 19, 2015

Member

If you have used add_on_empty its likely in a custom validation method with logic attached to it. That's probably why they didn't choose to use a validates_presence_of. Maybe combined with an :if option?

/cc @rafaelfranca

Member

senny commented Feb 19, 2015

If you have used add_on_empty its likely in a custom validation method with logic attached to it. That's probably why they didn't choose to use a validates_presence_of. Maybe combined with an :if option?

/cc @rafaelfranca

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 19, 2015

Member

Yeah, it is more a kind of internal method to be used in custom validations. I think it is better to suggest people to write the actual code:

errors.add(attribute, :empty, options) if value.nil? || value.empty?
Member

rafaelfranca commented Feb 19, 2015

Yeah, it is more a kind of internal method to be used in custom validations. I think it is better to suggest people to write the actual code:

errors.add(attribute, :empty, options) if value.nil? || value.empty?
@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Feb 19, 2015

Member

@rafaelfranca I changed it to your suggestion

Member

morgoth commented Feb 19, 2015

@rafaelfranca I changed it to your suggestion

@rafaelfranca

View changes

Show outdated Hide outdated activemodel/lib/active_model/errors.rb
@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Feb 19, 2015

Member

done

Member

morgoth commented Feb 19, 2015

done

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 19, 2015

Member

@morgoth thank you 💛

Member

senny commented Feb 19, 2015

@morgoth thank you 💛

@senny senny merged commit fd38838 into rails:master Feb 19, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

senny added a commit that referenced this pull request Feb 19, 2015

Merge pull request #18996 from morgoth/deprecate-more-errors-methods
Deprecate `ActiveModel::Errors` `add_on_empty` and `add_on_blank` methods

@morgoth morgoth deleted the morgoth:deprecate-more-errors-methods branch Mar 31, 2015

ledermann added a commit to ledermann/rails that referenced this pull request Aug 27, 2016

Fix typo in deprecation message
This fixes a copy-and-paste-issue slipped in by #18996
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment