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

Provide :touch option to save() to accommodate saving without updating t... #18225

Merged
merged 1 commit into from Dec 28, 2014

Conversation

@DanOlson
Copy link
Contributor

DanOlson commented Dec 27, 2014

...imestamps. [#18202]

@DanOlson DanOlson force-pushed the DanOlson:update-without-changing-timestamps branch Dec 28, 2014
@sgrif
sgrif reviewed Dec 28, 2014
View changes
activerecord/lib/active_record/persistence.rb Outdated
def save(*)
create_or_update
def save(**args)
create_or_update(args)

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

Let's just use normal splat here.

@sgrif
sgrif reviewed Dec 28, 2014
View changes
activerecord/lib/active_record/persistence.rb Outdated
def save!(*)
create_or_update || raise(RecordNotSaved.new(nil, self))
def save!(**args)
create_or_update(args) || raise(RecordNotSaved.new(nil, self))

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

Let's just use normal splat here.

@sgrif
sgrif reviewed Dec 28, 2014
View changes
activerecord/lib/active_record/persistence.rb Outdated
raise ReadOnlyRecord, "#{self.class} is marked as readonly" if readonly?
result = new_record? ? _create_record : _update_record
result = new_record? ? _create_record : _update_record(args)

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

Let's just use normal splat here.

@sgrif
sgrif reviewed Dec 28, 2014
View changes
activerecord/lib/active_record/timestamp.rb Outdated
@@ -58,7 +58,8 @@ def _create_record
end

def _update_record(*args)
if should_record_timestamps?
options = args.extract_options!

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

This can be written as def _update_record(*args, **options)

@sgrif
sgrif reviewed Dec 28, 2014
View changes
activerecord/lib/active_record/timestamp.rb Outdated
@@ -70,8 +71,10 @@ def _update_record(*args)
super
end

def should_record_timestamps?
self.record_timestamps && (!partial_writes? || changed?)
def should_record_timestamps?(**opts)

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

Let's call this options, since that's what we've called it elsewhere.

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

Actually why not just use a normal touch: keyword argument?

@sgrif
sgrif reviewed Dec 28, 2014
View changes
activerecord/lib/active_record/timestamp.rb Outdated
@@ -58,7 +58,8 @@ def _create_record
end

def _update_record(*args)
if should_record_timestamps?
options = args.extract_options!
if should_record_timestamps?(options)

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

How about instead of having options get passed to the predicate, we write this as:

def _update_record(touch: true)
  if touch && should_record_timestamps?

This comment has been minimized.

Copy link
@DanOlson

DanOlson Dec 28, 2014

Author Contributor

I like this, although the addition of keyword arguments to the method signature will require handling :validate as well. Additionally, we have to account for a possible positional argument, namely an array of model attributes.

The adjusted method signature would read as:

def _update_record(*args, touch: true, validate: true)

WDYT?

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

If we aren't using validate in this method, let's just ignore it.

def _update_record(*args, touch: true, **options)

This comment has been minimized.

Copy link
@DanOlson

DanOlson Dec 28, 2014

Author Contributor

Thanks a lot, Sean. I think I have everything addressed now.

This comment has been minimized.

Copy link
@DanOlson

DanOlson Dec 28, 2014

Author Contributor

Except for tests :P Standby...

@DanOlson DanOlson force-pushed the DanOlson:update-without-changing-timestamps branch Dec 28, 2014
@sgrif
Copy link
Contributor

sgrif commented Dec 28, 2014

Looks like the tests are failing.

@DanOlson DanOlson force-pushed the DanOlson:update-without-changing-timestamps branch Dec 28, 2014
@DanOlson
Copy link
Contributor Author

DanOlson commented Dec 28, 2014

Sorry, all green now. Had to adjust the signatures of the other methods in the inheritance chain.

@sgrif
sgrif reviewed Dec 28, 2014
View changes
activerecord/lib/active_record/attribute_methods/dirty.rb Outdated
def _update_record(*)
partial_writes? ? super(keys_for_partial_write) : super
def _update_record(*args, **options)
partial_writes? ? super(keys_for_partial_write, options) : super

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

Can you use the double splat here as well? Not strictly necessary, but will allow for potential performance improvements on the ruby side (skipping a hash allocation)

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

Actually could this be written as:

def _update_record(*args)
  partial_writes? ? super(keys_for_partial_write(*args) : super
@sgrif
sgrif reviewed Dec 28, 2014
View changes
activerecord/lib/active_record/locking/optimistic.rb Outdated
@@ -66,7 +66,7 @@ def increment_lock
send(lock_col + '=', previous_lock_value + 1)
end

def _update_record(attribute_names = self.attribute_names) #:nodoc:
def _update_record(attribute_names = self.attribute_names, **options) #:nodoc:

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 28, 2014

Contributor

Does this part of the chain need to care about keyword args? Can we just use normal splat?

This comment has been minimized.

Copy link
@DanOlson

DanOlson Dec 28, 2014

Author Contributor

Good point. I found I didn't need any of the keyword handling in most of the chain. Calling super *args in timestamp.rb removes the need. Thanks for the great feedback!

@sgrif
Copy link
Contributor

sgrif commented Dec 28, 2014

Last set of minor comments, then this is good to go

@DanOlson DanOlson force-pushed the DanOlson:update-without-changing-timestamps branch to e780e2f Dec 28, 2014
sgrif added a commit that referenced this pull request Dec 28, 2014
…tamps

Provide :touch option to save() to accommodate saving without updating t...
@sgrif sgrif merged commit 3ba552f into rails:master Dec 28, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@rafaelfranca
Copy link
Member

rafaelfranca commented Dec 29, 2014

What is the difference of this and no_touch?

@sgrif
Copy link
Contributor

sgrif commented Dec 29, 2014

@rafaelfranca
Copy link
Member

rafaelfranca commented Dec 29, 2014

hmmmm. At least we can make the implementation of both use the same thing? I dislike the idea of having the same idea implemented in two places.

@rafaelfranca
Copy link
Member

rafaelfranca commented Dec 29, 2014

Ah, no_touching is just when calling touch.

@DanOlson
Copy link
Contributor Author

DanOlson commented Dec 29, 2014

I can certainly look into that.

On Dec 29, 2014, at 10:57 AM, Rafael Mendonça França notifications@github.com wrote:

hmmmm. At least we can make the implementation of both use the same thing? I dislike the idea of having the same idea implemented in two places.


Reply to this email directly or view it on GitHub.

@sgrif
Copy link
Contributor

sgrif commented Dec 29, 2014

Also even if we did use the same implementation for both, almost all of the code which changed was for the signature, not the actual implementation of skipping the timestamps.

@DanOlson DanOlson deleted the DanOlson:update-without-changing-timestamps branch Dec 29, 2014
claudiob added a commit to claudiob/rails that referenced this pull request Dec 29, 2014
ActiveRecord::Base `save` and `save!` take an option boolean
`:touch` parameter since rails#18225 (stems from rails#18202).

This commit document that parameter.

[ci skip]
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016
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

3 participants
You can’t perform that action at this time.