-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add new Lint/DuplicateBranch
cop
#8404
Conversation
I thought we already had such a cop? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really should have a all_branches
(or just branches
) on if
and case
nodes, and you could have one single method that does check(all_branches.compact)
and that's it.
I think currently your logic can be simplified and would avoid potential issues. I didn't check, but I think:
case
when 1
foo
when 2
bar
else
bar # undetected
end
Also, stuff like this is not spec'ed:
case
when 1
42
else
42 # => offense
end
# but
case
when 1
42
when 2
42 # => no offense, is that what you want?
end
I had the same thought, but no.
Would you like to make a PR into
case
when 1
foo
when 2
bar
else
bar # undetected
end Do you mean that case
when 1
42
else
42 # => offense
end Yes, there is an offense in this case. # but
case
when 1
42
when 2
42 # => no offense, is that what you want?
end There is also an offense in this case too. |
What about
Sure
Ok
Are you sure? Won't it be excluded because of |
Both examples will be excluded because of # but
case
when 1
foo(42)
when 2
foo(42) # => no offense, is that what you want?
end And in this case it will detect offense. But for original example with basic literal as a body: # but
case
when 1
42
when 2
42 # => no offense, is that what you want?
end I think, I would prefer to not detect an offense because case
when :several, :grouped, :conditions
42
when :a, :couple, :of, :other, :related, :conditions
42
when :something, :else
50
end |
I realize I write sometimes too precisely, but please do not simply assume that I intended something (say So, let me repeat differently what I wrote (although I only read the code quickly and could very well be mistaken)
Also, the question with how this relates to |
Seems like that cop is a little bit poorly named. It checks if all branches have the same start line or end line, which can be extracted altogether from conditional. New cop checks if any two or more branches have identical bodies. It doesn't matter for |
Where do we stand with this cop? |
1f46da8
to
6359ae7
Compare
@marcandre Please, review this again.
But seems like it is not a good idea to find duplicated bodies amongst normal branches and else branch at the same time: if foo # 1. this condition is true
do_foo
elsif bar # 2. and this condition is true
do_bar
elsif baz
do_foo
else
do_foo
end If we deduplicate branches in this case and merge And when we have multiple branches with the same body as |
6359ae7
to
3eedd06
Compare
To me the first question is: what should be flag. Then we can ask what can we reasonably auto-correct (and not auto-correcting complex cases is fine by me). I imagine your above example has a typo, as the if !foo && bar
do_bar
else
do_foo
end |
5c96f43
to
33af048
Compare
Ok, I'm convinced. Updated the code to reflect that. Let's deliver this and see for some external feedback from other users? For CI to pass, we need rubocop/rubocop-ast#146 to be released. |
c6a17f3
to
1aa1e9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
|=== | ||
|
||
This cop checks that there are no repeated bodies | ||
within `if/unless`, `case-when` and `rescue` constructs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add some examples with case
and rescue
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
1aa1e9d
to
27ff0c6
Compare
config/default.yml
Outdated
Lint/DuplicateBranch: | ||
Description: Checks that there are no repeated bodies within `if/unless`, `case-when` and `rescue` constructs. | ||
Enabled: pending | ||
VersionAdded: '1.2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be 1.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, forgot to update this as well. So many new rubocop versions 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been on 🔥 lately 😉
27ff0c6
to
0f9d642
Compare
Thanks! |
What happens if the order of the branches matters? For example, I'm trying to determine what the commitment of a job listing is, based on its title. Here's a simplified example example: case title
when /full/i
Commitment::FULL_TIME
when /\bcontract|speculative|freelanc/i
Commitment::CONTRACTOR
when /\bpermanent\b|employee|\bhire/i
Commitment::FULL_TIME
end There are two duplicate code branches, but they serve a logical purpose: If the string matches on both In other words, some matches are more sure than others. This level of certainty is encoded in the order of the different branches. So while the contents may be duplicated, the order of those branches plays a vital role in the code's behavior. Is there a better way to write my code such that it doesn't violate this new RuboCop rule? (keep in mind that my actual code has a bunch more branches and commitment levels) Or should I just disable it here? |
It would probably be a good idea to accept a Either by default, or with a config option. |
Update: I understand now -- the I am getting a new warning for a case/else statement but its not obvious what rubocop is trying to say is wrong with my code: module AnnouncementsHelper
# Use the explicit class names so purgecss can find them
def announcement_color(announcement)
case announcement.kind
when "new"
"announcement-new"
when "update"
"announcement-update"
when "improvement"
"announcement-improvement"
when "fix"
"announcement-fix"
else
"announcement-update"
end
end It does not like the
If this case/else is bad, what should it be refactored to? |
@drnic I was going to reply, but then saw you already updated your own comment with the answer. One thing I'll add though, is that while technically your |
This cop somehow ignores the fact that |
This cop checks that there are no repeated bodies within
if/unless
,case-when
andrescue
constructs. Those can be as a result of refactoring, coding mistake or just badly repeated code.It does not consider simple repeated bodies, like basic literals, variable references, constant references and simple method calls without arguments. As otherwise this would force to probably make code uglier. Consider this example: https://github.com/rubocop-hq/rubocop/blob/974b2b1d28d66725407df9a9a0927ad50ced1d00/lib/rubocop/cop/layout/block_alignment.rb#L210-L217
Fun fact: I have unintentionally repeated branch within
on_if
method and this cop helped me to identify this 😄 So it is actually useful.Inspired by similar checks from 2 different java linters: https://pmd.github.io/latest/pmd_rules_java_codestyle.html#identicalcatchbranches and https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#db-method-uses-the-same-code-for-two-branches-db-duplicate-branches