actionpack: Fix helper and fragment_cache_key block to optional #715
Conversation
…che_key's block to optional and extends AbstractController::Caching::ClassMethods and AbstractController::Caching::Fragments::ClassMethods
|
@sanfrecce-osaka Thanks for your contribution! Please follow the instructions below for each change. Available commandsYou can use the following commands by commenting on this PR.
|
| include Caching | ||
| extend AbstractController::Caching::ClassMethods | ||
| extend AbstractController::Caching::Fragments::ClassMethods |
There was a problem hiding this comment.
It would be better to not only extend ClassMethods but also include AbstractController::Caching here.
ActionController::Caching dynamically calls include inside the included block. Therefore it's better to include it explicitly.
https://github.com/rails/rails/blob/v6.0.6.1/actionpack/lib/action_controller/base.rb#L219
https://github.com/rails/rails/blob/v6.0.6.1/actionpack/lib/action_controller/caching.rb#L28-L30
https://github.com/rails/rails/blob/v6.0.6.1/actionpack/lib/abstract_controller/caching.rb#L46
There was a problem hiding this comment.
Thank you for your feedback, I fixed it in 6b169ec
| # | ||
| # helper(:three, BlindHelper) { def mice() 'mice' end } | ||
| # | ||
| def helper: (*untyped args) ?{ () -> untyped } -> untyped |
There was a problem hiding this comment.
Could you move the methods only what you changed, please? If my understanding is correct, only ActionController::Helpers::ClassMethods#helper and AbstractController::Caching::Fragments::ClassMethods#fragment_cache_key should be moved and modified.
There was a problem hiding this comment.
Sorry, I wasn't sure how far I should move the auto-generated code, including comments, so I made sanfrecce-osaka@d7a4fb5 and sanfrecce-osaka@08b92b9 🙏
I fixed in 939ac11 based on your feedback.
…Caching cf. ruby#715 (comment) > It would be better to not only extend ClassMethods but also include `AbstractController::Caching` here. > > `ActionController::Caching` dynamically calls include inside the included block. Therefore it's better to include it explicitly. > > https://github.com/rails/rails/blob/v6.0.6.1/actionpack/lib/action_controller/base.rb#L219 https://github.com/rails/rails/blob/v6.0.6.1/actionpack/lib/action_controller/caching.rb#L28-L30 https://github.com/rails/rails/blob/v6.0.6.1/actionpack/lib/abstract_controller/caching.rb#L46
cf. ruby#715 (comment) > Could you move the methods only what you changed, please? If my understanding is correct, only `ActionController::Helpers::ClassMethods#helper` and `AbstractController::Caching::Fragments::ClassMethods#fragment_cache_key` should be moved and modified. revert d7a4fb5 and 08b92b9 excluding 7d95c0d and 1367681
| # | ||
| # helper(:three, BlindHelper) { def mice() 'mice' end } | ||
| # | ||
| def helper: (*untyped args) ?{ () -> untyped } -> untyped |
There was a problem hiding this comment.
Note: The above comment describes the argument are Module, Symbol, String. So it would be better to update signature like this:
| def helper: (*untyped args) ?{ () -> untyped } -> untyped | |
| def helper: (*Module | Symbol | String args) ?{ () -> void } -> void |
Either way, this is off-topic. I'll post another PR after merging this.
|
APPROVE |
|
Thanks! |
|
/merge |
|
This PR is not approved yet by the reviewers. Please get approval from the reviewers. See the Actions tab for detail. |
|
APPROVE |
|
Thanks for your review, @tk0miya! @sanfrecce-osaka, @tk0miya This PR is ready to be merged. |
|
Sorry for the confusion. I did not know the proper way to send APPROVE. |
|
/merge |
No description provided.