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

Rename method and stop passing unused arguements. #14466

Merged
merged 1 commit into from Mar 25, 2014

Conversation

vipulnsward
Copy link
Member

  • Renamed increment_or_decrement to an apt set_cache_value since it actually doesn't increment/decrement in localstore.
  • Stopped passing unused increment amount value to it.

@arthurnn
Copy link
Member

I am sure people extend that class, wondering if they rely on the amount value for something.

@vipulnsward
Copy link
Member Author

@arthurnn from the LocalStore class? They might but from the more generic Store class instead.

@rafaelfranca
Copy link
Member

What are the benefits of this change? As the code were before I could include local storage and override my increment_or_decrement method to do what I want receiving a amount value. Now I'd have to override the increment method to change it to pass the amount value.

@vipulnsward
Copy link
Member Author

@rafaelfranca that method is just an private internal helper method to re-use code.
To override behaviour for a store, user ideally would overwrite increment method, as done by all other cache store implementations. I have not changed the signature of increment method.

@rafaelfranca
Copy link
Member

Even with this I prefer to keep passing amount. A class including this module can choose to override only the private method. I'd change that method to protected

@vipulnsward
Copy link
Member Author

makes sense, will update.

…t actually doesn't increment/decrement in localstore.
@vipulnsward
Copy link
Member Author

@rafaelfranca updated.

rafaelfranca added a commit that referenced this pull request Mar 25, 2014
Rename method and stop passing unused arguements.
@rafaelfranca rafaelfranca merged commit e209723 into rails:master Mar 25, 2014
@vipulnsward vipulnsward deleted the rename-cache-method branch February 22, 2016 14:28
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.

None yet

3 participants