Follow DRY principle and remove duplication #21894
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Thanks, but we don't accept cosmetic changes. |
|
No problem :) |
|
I think it is worth to merge. I'll review it. |
| @@ -147,5 +135,9 @@ def optimize_routes_generation? #:nodoc: | |||
| def _generate_paths_by_default | |||
| true | |||
| end | |||
|
|
|||
| def get_only_path(host) | |||
There was a problem hiding this comment.
I think only_path? is better.
There was a problem hiding this comment.
Dislike get_ prefixes. Rename to only_path_for.
There was a problem hiding this comment.
Rafaels suggestion was clearer 👍
| @@ -147,5 +135,9 @@ def optimize_routes_generation? #:nodoc: | |||
| def _generate_paths_by_default | |||
| true | |||
| end | |||
|
|
|||
| def get_only_path(host) | |||
| host.nil? ? _generate_paths_by_default : false | |||
There was a problem hiding this comment.
Could save the method call and do host ? false : _generate_paths_by_default.
There was a problem hiding this comment.
I think if host is false we don't want to return false
There was a problem hiding this comment.
@abhishekjain16 this is something you should check first though. Change to @kaspth's suggestion and see if any test fail.
There was a problem hiding this comment.
@rafaelfranca @kaspth Great.
I will change method name and will be checking for return value.
abhishekjain16
force-pushed
the
refactor_with_dry
branch
from
365a12f
to
ce418e3
Compare
| @@ -147,5 +135,9 @@ def optimize_routes_generation? #:nodoc: | |||
| def _generate_paths_by_default | |||
| true | |||
| end | |||
|
|
|||
| def only_path_for(host) | |||
There was a problem hiding this comment.
I still think only_path? is better 😁
There was a problem hiding this comment.
@kaspth Though this method name seems good considering it returns boolean value for now, but what if in future _generate_paths_by_default returns some other value rather than boolean?
Should we still go with only_path?
What do you think?
There was a problem hiding this comment.
if that happen we rename the method again. For now let's use only_path?
abhishekjain16
force-pushed
the
refactor_with_dry
branch
from
ce418e3
to
9933fc1
Compare
|
@rafaelfranca Done 👍 |
| options[:only_path] = false | ||
| end | ||
| end | ||
| options[:only_path] = only_path?(options[:host]) unless options.key?(:only_path) |
There was a problem hiding this comment.
It is still missing the change of unless in the beginning of the line
abhishekjain16
force-pushed
the
refactor_with_dry
branch
from
9933fc1
to
9a57e7f
Compare
|
@rafaelfranca Sorry. My bad. Change done now 👍 |
Follow DRY principle and remove duplication
No description provided.