Conversation
|
I don't understand why codecov/patch failed here. |
|
I think it's been a big coincidence that the coverage of your patch matches the global coverage of the project 😲. Probably your patch loses at the third decimal digit. I normally use a more relaxed configuration for this in |
|
Yeah, getting the patch 100% covered is another option 😃. |
|
By the way, codeclimate just rolled out their out code coverage status checks in PR, and they chose 80% as a default for this setting: https://codeclimate.com/changelog/59baa212ced24c026c0017b8. |
Fivell
left a comment
There was a problem hiding this comment.
I started review commit by commit , but in the end looks like all my comments were fixed
| end | ||
|
|
||
| # Deals with +ControllerAction+ instances | ||
| # Builds one route for each HTTP verb passed in |
There was a problem hiding this comment.
@varyonic , I think this methods should be tested or defined as private
lib/active_admin/router.rb
Outdated
| router.collection do | ||
| config.collection_actions.each { |action| build_action(router, action) } | ||
| router.post :batch_action if config.batch_actions_enabled? | ||
| end |
| router.member do | ||
| config.member_actions.each { |action| build_action(router, action) } | ||
| end | ||
|
|
lib/active_admin/router.rb
Outdated
| # | ||
| # @param router [ActionDispatch::Routing::Mapper] | ||
| def apply(router) | ||
| def apply(router = @router) |
There was a problem hiding this comment.
@varyonic, not sure this commit makes code more cleaner, I don't understand now if there there are cases when different routers can be used for initialize and apply method ? in other words after router passed to constructor, why this method needs argument
There was a problem hiding this comment.
@varyonic ignore this since it was fixed by next commit
deivid-rodriguez
left a comment
There was a problem hiding this comment.
@varyonic This refactoring is absolutely fantastic! Just made a couple of comments on naming, but I'm super happy seeing this land into master anyways!
|
|
||
| instance_exec &routes | ||
| end | ||
| def define_routes(config) |
There was a problem hiding this comment.
All of these methods are defining routes, could this method name be more clear?
| if config.belongs_to? | ||
| define_belongs_to_routes(config) | ||
| else | ||
| page_or_resource_routes(config) |
There was a problem hiding this comment.
For consistency, should these methods be named either define_belongs_to_routes & define_page_or_resource_routes, or belongs_to_routes & page_or_resource_routes?
There was a problem hiding this comment.
I agree the method naming is not great, I was focusing on unfolding the code and followed the examples of #3170. Renaming methods retroactively with an interactive rebase can get tricky so I'll look at a final commit to do this.
There was a problem hiding this comment.
On second thoughts reviewing method names just left me wanting to refactor even further. I'ld rather close this technical excision of proc/exec and leave further improvements to a separate PR.
|
Thanks to @dmitry for showing this could be done. |
Refactor Router
This series of refactorings, heavily inspired by #3170, replaces all use of proc (x5) and instance_exec (x6) in Router with vanilla method calls, making it easier to understand and maintain.