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

Counter cache column #619

Merged
merged 1 commit into from
Mar 9, 2023
Merged

Conversation

kbrock
Copy link
Collaborator

@kbrock kbrock commented Mar 4, 2023

code was pulled out of #600

  • Drop unnecessary _counter_cache_column, and remove all the to_s that were used.

May merge with #618 - but at this time, refactoring the code and fixing the counters that get askew seem like different problems

@kbrock kbrock force-pushed the counter_cache_column branch 2 times, most recently from 2a6e976 to def1a66 Compare March 5, 2023 03:16
@kbrock
Copy link
Collaborator Author

kbrock commented Mar 5, 2023

@kshnurov A few questions:

  • was there a particular use case you were protecting by adding the parent_id checks?
  • It looks like those checks are already made in the callback definitions.

I did notice that ancestry_callbacks_disabled? was not checked in the increase_parent_counter_cache. Not sure if these are the root of #618. Will probably move from this PR and put into that one since it is more of a potential bug fix rather than a refactor.

@kbrock kbrock marked this pull request as ready for review March 5, 2023 03:20
@kbrock kbrock mentioned this pull request Mar 6, 2023
@kbrock kbrock force-pushed the counter_cache_column branch 2 times, most recently from dd22476 to de525a7 Compare March 9, 2023 22:27
The value is available, no reason to define a duplicate method
@kbrock kbrock force-pushed the counter_cache_column branch from de525a7 to ff8ee39 Compare March 9, 2023 22:36
@kbrock
Copy link
Collaborator Author

kbrock commented Mar 9, 2023

update:

@kbrock kbrock merged commit 8b71773 into stefankroes:master Mar 9, 2023
@kbrock kbrock deleted the counter_cache_column branch March 9, 2023 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant