Add AR::Base#valid! method #8639

Merged
merged 1 commit into from Jun 27, 2014

Conversation

Projects
None yet
10 participants
@bogdan
Contributor

bogdan commented Dec 28, 2012

This is helper method in case when some operations should be done on a record before saving. But they doesn't make sence if record is invalid.

def create_developer_from_linkedin(parameters)
  d = Developer.new(parameters)
  d.valid!
  d.import_linkedin_profile
  d.save!
end
@rrouse

This comment has been minimized.

Show comment
Hide comment
@rrouse

rrouse Dec 28, 2012

Why would this be preferred over just doing "if valid?...."?

rrouse commented Dec 28, 2012

Why would this be preferred over just doing "if valid?...."?

@gmile

This comment has been minimized.

Show comment
Hide comment
@gmile

gmile Dec 28, 2012

Contributor

Indeed, @bogdan, can you show a case where #valid? wouldn't be sufficient?

Contributor

gmile commented Dec 28, 2012

Indeed, @bogdan, can you show a case where #valid? wouldn't be sufficient?

@PikachuEXE

This comment has been minimized.

Show comment
Hide comment
@PikachuEXE

PikachuEXE Jan 7, 2013

Contributor

@bogdan I can rewrite the code provided with this:

def create_developer_from_linkedin(parameters)
  d = Developer.new(parameters)
  if d.valid?
    d.import_linkedin_profile
    d.save!
  end
  # d # if you still want the invalid record returned
end

So you might want to provide another example where valid? does not work but valid! does

Contributor

PikachuEXE commented Jan 7, 2013

@bogdan I can rewrite the code provided with this:

def create_developer_from_linkedin(parameters)
  d = Developer.new(parameters)
  if d.valid?
    d.import_linkedin_profile
    d.save!
  end
  # d # if you still want the invalid record returned
end

So you might want to provide another example where valid? does not work but valid! does

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Jan 7, 2013

Contributor

I need to raise exception instead of returning something. This required me to reverse engineer AR::RecordInvalid and see in source how it works. Currently my code looks like this:

def create_developer_from_linkedin(parameters)
  d = Developer.new(parameters)
  if d.valid?
    d.import_linkedin_profile
    d.save!
  else
    raise ActiveRecord::RecordInvalid.new(d)
  end
end
Contributor

bogdan commented Jan 7, 2013

I need to raise exception instead of returning something. This required me to reverse engineer AR::RecordInvalid and see in source how it works. Currently my code looks like this:

def create_developer_from_linkedin(parameters)
  d = Developer.new(parameters)
  if d.valid?
    d.import_linkedin_profile
    d.save!
  else
    raise ActiveRecord::RecordInvalid.new(d)
  end
end
@PikachuEXE

This comment has been minimized.

Show comment
Hide comment
@PikachuEXE

PikachuEXE Jan 7, 2013

Contributor

I think you might want to post the code the make use of the exception raised
Then we can start discussing whether the exception is needed or not

Contributor

PikachuEXE commented Jan 7, 2013

I think you might want to post the code the make use of the exception raised
Then we can start discussing whether the exception is needed or not

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Jan 7, 2013

Contributor

@PikachuEXE There are a lot of factors why it's raised including subjective ones like "someone did that long time ago with 0% test coverage". I don't want to explain those details. But I think rails should be flexible enough to give a developer ability to choose between raise exception or return value.

Contributor

bogdan commented Jan 7, 2013

@PikachuEXE There are a lot of factors why it's raised including subjective ones like "someone did that long time ago with 0% test coverage". I don't want to explain those details. But I think rails should be flexible enough to give a developer ability to choose between raise exception or return value.

@PikachuEXE

This comment has been minimized.

Show comment
Hide comment
@PikachuEXE

PikachuEXE Jan 8, 2013

Contributor

Well since rails 4 are having new destroy! and other bang methods
I think adding this method should be ok
But the name might better be validate! instead of valid!

Anyway it's up to the core members to decide.

Contributor

PikachuEXE commented Jan 8, 2013

Well since rails 4 are having new destroy! and other bang methods
I think adding this method should be ok
But the name might better be validate! instead of valid!

Anyway it's up to the core members to decide.

@timraymond

This comment has been minimized.

Show comment
Hide comment
@timraymond

timraymond Jan 25, 2013

Contributor

I believe this needs a CHANGELOG entry. Also, could you please document valid!

Contributor

timraymond commented Jan 25, 2013

I believe this needs a CHANGELOG entry. Also, could you please document valid!

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 25, 2013

Member

We are going to review all this API in the next release so we'll not accept this right now.

I'll leave this issue open to remember.

Member

rafaelfranca commented Jan 25, 2013

We are going to review all this API in the next release so we'll not accept this right now.

I'll leave this issue open to remember.

@ghost ghost assigned rafaelfranca Jan 25, 2013

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Mar 2, 2013

Contributor

Now that the Rails 4 beta is out, has this been reviewed?

Contributor

JonRowe commented Mar 2, 2013

Now that the Rails 4 beta is out, has this been reviewed?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 30, 2013

Member

Yeah, we will revisit this one form 4.1

Member

rafaelfranca commented Apr 30, 2013

Yeah, we will revisit this one form 4.1

@vipulnsward

This comment has been minimized.

Show comment
Hide comment
@vipulnsward

vipulnsward Aug 27, 2013

Member

@rafaelfranca This can be reviewed now I guess?

Member

vipulnsward commented Aug 27, 2013

@rafaelfranca This can be reviewed now I guess?

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Sep 7, 2013

Contributor

I think .validate! makes more sense than .valid!. But I agree it's a nice PR.

I wonder if save! can be refactored to just call validate! then. Essentially no new code would be added, just the invalid error raising message would be pushed from .save! method to .validate! method.

Contributor

egilburg commented Sep 7, 2013

I think .validate! makes more sense than .valid!. But I agree it's a nice PR.

I wonder if save! can be refactored to just call validate! then. Essentially no new code would be added, just the invalid error raising message would be pushed from .save! method to .validate! method.

@mikegee

This comment has been minimized.

Show comment
Hide comment
@mikegee

mikegee Jun 21, 2014

Contributor

This is quite stale now. What is the current blocker to merging this?

I also prefer #validate! over #valid!.

Contributor

mikegee commented Jun 21, 2014

This is quite stale now. What is the current blocker to merging this?

I also prefer #validate! over #valid!.

Add AR::Base#validate! method
Acts same as valid? but raises AR::RecordInvalid exception
if validation fails
@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Jun 23, 2014

Contributor

Renamed method to validate! as most people voted for it.
@rafaelfranca waiting for your review whenever you have time.

Contributor

bogdan commented Jun 23, 2014

Renamed method to validate! as most people voted for it.
@rafaelfranca waiting for your review whenever you have time.

@rafaelfranca rafaelfranca merged commit 0fa4c95 into rails:master Jun 27, 2014

1 check passed

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

rafaelfranca added a commit that referenced this pull request Jun 27, 2014

@bogdan bogdan deleted the bogdan:valid-with-bang branch Jan 29, 2016

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