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#touch should accept multiple attributes #14423

Merged

Conversation

@thejamespinto
Copy link
Contributor

thejamespinto commented Mar 19, 2014

No description provided.

@thejamespinto
Copy link
Contributor Author

thejamespinto commented Mar 19, 2014

I wrote an extra test too

@carlosantoniodasilva
Copy link
Member

carlosantoniodasilva commented Mar 19, 2014

Mind explaining a use case for many attributes being updated? Thanks.

@thejamespinto
Copy link
Contributor Author

thejamespinto commented Mar 19, 2014

this github page is a good example

When you press "close & comment",
I should update both "closed_at" and "last_commented_at" attributes

@egilburg
egilburg reviewed Mar 19, 2014
View changes
activerecord/lib/active_record/persistence.rb Outdated
raise ActiveRecordError, "cannot touch on a new record object" unless persisted?

attributes = timestamp_attributes_for_update_in_model
attributes << name if name
attributes += names

This comment has been minimized.

Copy link
@egilburg

egilburg Mar 19, 2014

Contributor

Can use attributes.concat(names) to avoid extra object allocation.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Mar 19, 2014

Member

And we should do this only if we have any name

This comment has been minimized.

Copy link
@thejamespinto

thejamespinto Mar 19, 2014

Author Contributor

@egilburg I just bm'd it to 100_000_000 times,
the results show a 0.5 .. 1.3% performance improvement,
thanks for the tip 👍

raise ActiveRecordError, "cannot touch on a new record object" unless persisted?

attributes = timestamp_attributes_for_update_in_model
attributes << name if name
attributes.concat(names)

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Mar 19, 2014

Member

We need to check if names is not empty.

This comment has been minimized.

Copy link
@thejamespinto

thejamespinto Mar 19, 2014

Author Contributor

@rafaelfranca, I don't think it's necessary.
while using Array#concat, Array#empty? shows no difference performance penalties or benefits
Array#any? should be avoided in a Rails environment
image 2014-03-19 at 5 01 14 pm

@rafaelfranca
Copy link
Member

rafaelfranca commented Mar 19, 2014

Could you add a CHANGELOG entry?

@egilburg
Copy link
Contributor

egilburg commented Mar 19, 2014

@yakko if we'd be getting into microoptimization, unless b.empty? would be slightly faster than if !b.empty? because ! is a method call in Ruby. But I don't think it'll be a meaningful difference.

@thejamespinto
Copy link
Contributor Author

thejamespinto commented Mar 19, 2014

@rafaelfranca
Copy link
Member

rafaelfranca commented Mar 19, 2014

Could you squash your commits?

@thejamespinto
Copy link
Contributor Author

thejamespinto commented Mar 19, 2014

squashed it is

@carlosantoniodasilva
carlosantoniodasilva reviewed Mar 20, 2014
View changes
activerecord/CHANGELOG.md Outdated
photo.sealed_at # was updated

*James Pinto*

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva Mar 20, 2014

Member

One last thing: this entry should be at the top. Thanks!

@thejamespinto
Copy link
Contributor Author

thejamespinto commented Mar 20, 2014

@rafaelfranca @carlosantoniodasilva the conditions were met and TravisCI has passed 👍

carlosantoniodasilva added a commit that referenced this pull request Mar 20, 2014
ActiveRecord#touch should accept multiple attributes

Conflicts:
	activerecord/CHANGELOG.md
@carlosantoniodasilva carlosantoniodasilva merged commit c80ca4c into rails:master Mar 20, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
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

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