Skip to content
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

ActiveRecord/ActiveModel '#validate' alias for 'valid?' #14456

Merged
merged 1 commit into from Mar 27, 2014

Conversation

@henrik
Copy link
Contributor

@henrik henrik commented Mar 23, 2014

It's unintuitive to call '#valid?' when you want to run validations but don't care about the return value, e.g. in a test:

x.name  = ""
x.valid?
assert_not_empty x.errors_on(:name)

Compare that to

x.name  = ""
x.validate
assert_not_empty x.errors_on(:name)

The alias in ActiveRecord isn't strictly necessary (the ActiveModel alias is still in effect), but it clarifies.

@dhh was positive when I asked a few months ago: https://twitter.com/henrik/status/390205394305703936

An alternative implementation would be to actually implement valid? in terms of validate, but I figured an alias would be the simplest thing that works.

@chancancode
Copy link
Member

@chancancode chancancode commented Mar 23, 2014

While I understand that it reads nicer in certain less common cases, but I'm not sure if the benefits are worth it.

The reason I'm on the cautious side is because every method we add to the Active Record API increases comes with the risk of conflict with user code. If we add this method, you will no longer be able to have db columns called validate or define custom methods on your model called validate.

While I agree the former is quite unlikely, I think it is quite possible that we will cause problems for existing apps/gems that defines a validate method on their models with different signature/semantics/side-effects.

For example, some apps might have defined BillingMethod#validate that hits an external service. If this becomes part of the public API and future framework code or gems starts calling this method, their app might suddenly become very slow because of those unintended service calls.

This is not a particular high risk name for us to use, but the benefits seems pretty insignificant either. Personally I'd err on the cautious side, but I'll defer to @dhh to weigh the pros/cons.

@dhh
Copy link
Member

@dhh dhh commented Mar 23, 2014

I don't think it's a problem to add this as an alias because it happens to the base class. So if you have your own validate method, you'll just be overwriting the method. As long as we are not using the method internally.

On Mar 23, 2014, at 13:11, Godfrey Chan notifications@github.com wrote:

While I understand that it reads nicer in certain less common cases, but I'm not sure if the benefits are worth it.

The reason I'm on the cautious side is because every method we add to the Active Record API increases comes with the risk of conflict with user code. If we add this method, you will no longer be able to have db columns called validate or define custom methods on your model called validate.

While I agree the former is quite unlikely, I think it is quite possible that we will cause problems for existing apps/gems that defines a validate method on their models with different signature/semantics/side-effects.

For example, some apps might have defined BillingMethod#validate that hits an external service. If this becomes part of the public API and future framework code or gems starts calling this method, their app might suddenly become very slow because of those unintended service calls.

This is not a particular high risk name for us to use, but the benefits seems pretty insignificant either. Personally I'd err on the cautious side, but I'll defer to @dhh to weigh the pros/cons.


Reply to this email directly or view it on GitHub.

assert_empty r.errors[:author_name]

r.validate(:special_case)
refute_empty r.errors[:author_name]

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Mar 23, 2014
Member

There's assert_not_empty I guess you could use, we try to avoid refuteusage.

This comment has been minimized.

@henrik

henrik Mar 23, 2014
Author Contributor

Excellent; I like that better. Pushing a test cleanup.

This comment has been minimized.

@dhh

dhh Mar 23, 2014
Member

Actually, I'm starting to have second thoughts if the only use case we can think for this is tests. These tests would have been just as well if you had a assert_not r.valid? before your error check. Do you have a non-test example?

On Mar 23, 2014, at 16:45, Henrik Nyh notifications@github.com wrote:

In activerecord/test/cases/validations_test.rb:

@@ -52,6 +52,21 @@ def test_error_on_given_context
assert r.save(:context => :special_case)
end

  • def test_validate
  • r = WrongReply.new(:title => "Valid title")
  • r.validate
  • assert_empty r.errors[:author_name]
  • r.validate(:special_case)
  • refute_empty r.errors[:author_name]
    Excellent; I like that better. Pushing a test cleanup.


Reply to this email directly or view it on GitHub.

@carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Mar 23, 2014

Just as a side note, thinking about your example, in general I add an assertion to the valid? call too, so it looks just fine: assert r.valid?.

@henrik
Copy link
Contributor Author

@henrik henrik commented Mar 23, 2014

I got a comment by email (that I don't see on this pull request for some reason) – whether I have more use cases than testing validations per above.

Grepping through one of our apps for uses of valid? without a conditional:

I see some non-test code that collects errors on several records by calling the_record.valid? on each and concatenating.

There's also some non-test code where one controller (as part of an unconventional flow) is passed a set of attributes and shows a form – it validates to show errors, if any, but doesn't do anything conditionally based on the validation result.

I also see several cases of testing validation callbacks: foo.valid?; expect(foo).to show_the_effect_of_the_callback. These are callbacks that e.g. normalize or cache some value before validation. The tests would communicate their intent a lot better if they did foo.validate instead of foo.valid? or assert foo.valid? (the latter actually distracts from the intent of the test).

Examples aside, I think a method like this makes a lot of sense in the API. I know I've wanted this method several times in the past.

@billhorsman
Copy link

@billhorsman billhorsman commented Mar 23, 2014

I too dislike having to call valid? when I don't care about the result. I think there is a risk that someone could come back and look at that code and think it's redundant. A name that describes the intention would be nicer.

As a side issue, I don't usually use assert r.valid? because if that fails then I don't know why. I prefer to use my own custom (MiniTest) validation:

  def assert_valid(model)
    model.valid?
    assert model.errors.to_a == [], "Unexpected errors: #{model.errors.to_a}"
  end

I also have:

  def refute_valid(model)
    model.valid?
    refute model.errors.to_a == [], "Expected errors but found none"
  end

  def assert_errors(messages, model)
    model.valid?
    assert model.errors.to_a.sort == messages.sort, "Expected: #{messages.sort.inspect}\nActual: #{model.errors.to_a.sort.inspect}"
  end

  def assert_includes_error(message, model)
    model.valid?
    assert model.errors.to_a.include?(message), "Expected #{model.errors.to_a.inspect} to include #{message}"
  end

All of them would be improved by using model.validate instead. +1

@dhh
Copy link
Member

@dhh dhh commented Mar 23, 2014

Aight, I buy the collection case.

On Mar 23, 2014, at 7:59 PM, Henrik Nyh notifications@github.com wrote:

I got a comment by email (that I don't see on this pull request for some reason) – whether I have more use cases than testing validaitons.

Grepping through one of our apps for uses of valid? without a conditional:

I see some code that collects errors on several records by calling the_record.valid? on each and concatenating.

I also see a test for a validation callback: foo.valid?; expect(foo).to show_the_effect_of_the_callback. E.g. normalizing or caching some value before validation. The tests would communicate their intent a lot better if they did foo.validate instead of foo.valid? or assert foo.valid? (the latter actually distracts from the intent of the test).

Examples aside, I think a method like this makes a lot of sense in the API. I know I've wanted this method several times in the past.


Reply to this email directly or view it on GitHub.

@henrik
Copy link
Contributor Author

@henrik henrik commented Mar 25, 2014

Let me know if there's anything else I can do towards getting this merged.

@egilburg
Copy link
Contributor

@egilburg egilburg commented Mar 27, 2014

Semantically it would seem to me that validate should run the validation and then return self rather than true or false

@henrik
Copy link
Contributor Author

@henrik henrik commented Mar 27, 2014

I implemented something along those lines first, but I figured it might be better to do something minimal.

Also consider that e.g. 'save' returns a bool, so the current behavior has precedents.

Sent from my iPhone

On 27 Mar 2014, at 08:23, Eugene Gilburg notifications@github.com wrote:

Semantically it would seem to me that validate should run the validation and then return self rather than true or false


Reply to this email directly or view it on GitHub.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 27, 2014

Could you add a CHANGELOG entry?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 27, 2014

And also please squash your commits

@henrik
Copy link
Contributor Author

@henrik henrik commented Mar 27, 2014

Will do! squash and then force push this branch, right?

Sent from my iPhone

On 27 Mar 2014, at 14:36, Rafael Mendonça França notifications@github.com wrote:

And also please squash your commits


Reply to this email directly or view it on GitHub.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 27, 2014

Right

It's unintuitive to call '#valid?' when you want to run validations but
don't care about the return value.

The alias in ActiveRecord isn't strictly necessary (the ActiveModel
alias is still in effect), but it clarifies.
@henrik
Copy link
Contributor Author

@henrik henrik commented Mar 27, 2014

Added CHANGELOG entry, rebased against upstream master, squashed.

The ActiveModel CHANGELOG just says "Please check 4-1-stable for previous changes." so I'm not sure what to do there. Left it alone until I hear differently.

rafaelfranca added a commit that referenced this pull request Mar 27, 2014
ActiveRecord/ActiveModel '#validate' alias for 'valid?'
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 27, 2014

Merged. Thank you so much

@rafaelfranca rafaelfranca merged commit 2e70f44 into rails:master Mar 27, 2014
1 check was pending
1 check was pending
@jeremy
default The Travis CI build is in progress
Details
@henrik
Copy link
Contributor Author

@henrik henrik commented Mar 27, 2014

@rafaelfranca Thanks so much for the feedback and for merging!

@bogdan
Copy link
Contributor

@bogdan bogdan commented May 2, 2014

but don't care about the return value

It would be so cool if this method would return self if you don't care about returning value. I would use it for a different purpose - not just tests

@henrik
Copy link
Contributor Author

@henrik henrik commented May 2, 2014

@bogdan What would you use it for?

@bogdan
Copy link
Contributor

@bogdan bogdan commented May 2, 2014

I've written about it here: #8639
Method you've build is a little different, so suppose we have a method that builds an AR::Base object and returns it without saving in the database.

In this case method could look like this:

def build_user
  user = User.new
  # additional manipulations
  user.validate
end
@henrik
Copy link
Contributor Author

@henrik henrik commented May 2, 2014

Aha, I think I understand.

We probably shouldn't continue this discussion here (since the pullreq is merged and notifications may go out to commenters), but I suggest making your own pull request and/or starting a discussion on the rubyonrails-core mailing list. Then post the link here for interested parties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants