Add increment_counter!, decrement_counter!, update_counters! #12036

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@ctide

ctide commented Aug 26, 2013

These will be used to automatically update the updated_at field as well as modify whichever counters have been specified.

@robin850

View changes

activerecord/lib/active_record/counter_cache.rb
+ end
+
+ private
+ def updates(counters)

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Aug 27, 2013

Member

Could you please indent this method definition as the coding conventions states ?

@robin850

robin850 Aug 27, 2013

Member

Could you please indent this method definition as the coding conventions states ?

@robin850

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Aug 27, 2013

Member

Thanks for your contribution! The implementation looks fine to me but your tests are failing.

Moreover, the documentation is awesome but maybe we should move it to non-bang methods and just state This method differs from it's non-bang version in that it also sets updated_at to now. for bang ones, what do you think ?

Member

robin850 commented Aug 27, 2013

Thanks for your contribution! The implementation looks fine to me but your tests are failing.

Moreover, the documentation is awesome but maybe we should move it to non-bang methods and just state This method differs from it's non-bang version in that it also sets updated_at to now. for bang ones, what do you think ?

@ctide

This comment has been minimized.

Show comment Hide comment
@ctide

ctide Aug 27, 2013

Sure, I slimmed up the documentation (the non-bang ones already have full documentation, I just had copied it over. I replaced the bang documentation with just the difference between that and the regular version.) I thought those tests might be a little racey, but they had been passing every time on my machine. I've made them more explicit now.

ctide commented Aug 27, 2013

Sure, I slimmed up the documentation (the non-bang ones already have full documentation, I just had copied it over. I replaced the bang documentation with just the difference between that and the regular version.) I thought those tests might be a little racey, but they had been passing every time on my machine. I've made them more explicit now.

@robin850

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Aug 27, 2013

Member

Sure, I slimmed up the documentation (the non-bang ones already have full documentation, I just had copied it over. I replaced the bang documentation with just the difference between that and the regular version.)

Oh yes, right! Sorry, didn't noticed that the diff was trimmed so I thought there was only a single line for non-bang methods's documentation. However, I don't think that it's a good idea to duplicate the documentation like this. We should better put a reference to the non-bang version.

I thought those tests might be a little racey, but they had been passing every time on my machine. I've made them more explicit now.

Seems that they pass for PostgreSQL and SQLite3 but not for MySQL and MYSQL 2. Could you have a look please ? 😄

Member

robin850 commented Aug 27, 2013

Sure, I slimmed up the documentation (the non-bang ones already have full documentation, I just had copied it over. I replaced the bang documentation with just the difference between that and the regular version.)

Oh yes, right! Sorry, didn't noticed that the diff was trimmed so I thought there was only a single line for non-bang methods's documentation. However, I don't think that it's a good idea to duplicate the documentation like this. We should better put a reference to the non-bang version.

I thought those tests might be a little racey, but they had been passing every time on my machine. I've made them more explicit now.

Seems that they pass for PostgreSQL and SQLite3 but not for MySQL and MYSQL 2. Could you have a look please ? 😄

@ctide

This comment has been minimized.

Show comment Hide comment
@ctide

ctide Aug 27, 2013

Should work across the board now for all various connection types.

ctide commented Aug 27, 2013

Should work across the board now for all various connection types.

@robin850

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Aug 27, 2013

Member

Thanks a lot, could you please also remove the example in the method's documentation ? It's very kind to try to enhance our documentation and we really appreciate but our guidelines discourage the usage of gratuitous "Examples" sections like those.

Also the build has failed. 😄

Member

robin850 commented Aug 27, 2013

Thanks a lot, could you please also remove the example in the method's documentation ? It's very kind to try to enhance our documentation and we really appreciate but our guidelines discourage the usage of gratuitous "Examples" sections like those.

Also the build has failed. 😄

@ctide

This comment has been minimized.

Show comment Hide comment
@ctide

ctide Aug 27, 2013

Removed the examples. That build should fail, it was from the pre-rebased version of the tests. This will be the relevant build as it's tied to the most recent rebasing of this pull request.

ctide commented Aug 27, 2013

Removed the examples. That build should fail, it was from the pre-rebased version of the tests. This will be the relevant build as it's tied to the most recent rebasing of this pull request.

@ctide

This comment has been minimized.

Show comment Hide comment
@ctide

ctide Aug 27, 2013

Looks like the most recent build passed, let me know if you have any other feedback.

ctide commented Aug 27, 2013

Looks like the most recent build passed, let me know if you have any other feedback.

@thibaudgg

View changes

activemodel/CHANGELOG.md
@@ -1,3 +1,9 @@
+* Add `increment_counters!`, `decrement_counters!`, and `update_counters!`

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Sep 6, 2013

Contributor

increment_counters! and decrement_counters! should not use plural form => increment_counter! and decrement_counter!

Useful update, thanks!

@thibaudgg

thibaudgg Sep 6, 2013

Contributor

increment_counters! and decrement_counters! should not use plural form => increment_counter! and decrement_counter!

Useful update, thanks!

This comment has been minimized.

Show comment Hide comment
@ctide

ctide Sep 6, 2013

Will be fixed momentarily, thanks for pointing it out!

@ctide

ctide Sep 6, 2013

Will be fixed momentarily, thanks for pointing it out!

@jbaudanza

This comment has been minimized.

Show comment Hide comment
@jbaudanza

jbaudanza Sep 27, 2013

Contributor

👍

Contributor

jbaudanza commented Sep 27, 2013

👍

@robin850

This comment has been minimized.

Show comment Hide comment
@robin850

robin850 Sep 27, 2013

Member

Sorry, I'm not a merger and for now the core team is quite busy. I will try to ask someone to review this one. Anyway, I've noticed that your changelog entry is in the ActiveModel changelog file, could you move it to ActiveRecord please ?

Member

robin850 commented Sep 27, 2013

Sorry, I'm not a merger and for now the core team is quite busy. I will try to ask someone to review this one. Anyway, I've noticed that your changelog entry is in the ActiveModel changelog file, could you move it to ActiveRecord please ?

@ctide

This comment has been minimized.

Show comment Hide comment
@ctide

ctide Sep 27, 2013

Moved it to the correct changelog file.

ctide commented Sep 27, 2013

Moved it to the correct changelog file.

@rafaelfranca

View changes

activerecord/lib/active_record/counter_cache.rb
+ #
+ def update_counters!(id, counters)
+ quoted_column = connection.quote_column_name('updated_at')
+ now = connection.quote(connection.quoted_date(Time.now))

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca May 24, 2014

Member

We should use current_time_from_proper_timezone instead of Time.now

@rafaelfranca

rafaelfranca May 24, 2014

Member

We should use current_time_from_proper_timezone instead of Time.now

@rafaelfranca

View changes

activerecord/lib/active_record/counter_cache.rb
+ # This method differs from its non-bang version in that it also sets updated_at to now.
+ #
+ def update_counters!(id, counters)
+ quoted_column = connection.quote_column_name('updated_at')

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca May 24, 2014

Member

We should use timestamp_attributes_for_update_in_model instead of updated_at only. And only write if the columns exists.

@rafaelfranca

rafaelfranca May 24, 2014

Member

We should use timestamp_attributes_for_update_in_model instead of updated_at only. And only write if the columns exists.

This comment has been minimized.

Show comment Hide comment
@ctide

ctide May 27, 2014

What would you expect to happen if the columns don't exist? That it should just silently act like update_attributes instead of update_attributes!?

@ctide

ctide May 27, 2014

What would you expect to happen if the columns don't exist? That it should just silently act like update_attributes instead of update_attributes!?

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca May 27, 2014

Member

Yes. I think it is better.

@rafaelfranca

rafaelfranca May 27, 2014

Member

Yes. I think it is better.

@ctide

This comment has been minimized.

Show comment Hide comment
@ctide

ctide May 27, 2014

OK, I've updated this to pull from those methods instead as well as pulling in the latest changes from rails/master

ctide commented May 27, 2014

OK, I've updated this to pull from those methods instead as well as pulling in the latest changes from rails/master

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014

@ctide ctide closed this Dec 14, 2015

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

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