-
-
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 IgnoreLiteralBranches
and IgnoreConstantBranches
options to Lint/DuplicateBranch
#9193
Conversation
d451694
to
2ed842a
Compare
I'm fine with the proposed change, although we might want to limit it only to |
It's great you've spotted this, but it's a pitty they don't reach out to share their concerns directly. |
Yeah even though I wouldn't want to be forced into a config personally, I take a peek there every once in a while to see if there's anything we can fix. |
2ed842a
to
9e694b6
Compare
@bbatsov updated, |
What I dislike about that cop is that it doesn't differentiate between simple literals and the rest.} E.g.: I think this code would benefit from a refactor: case size
when "small" then do_something(100)
when "medium" then do_something_else(250)
when "large" then extra_processing
else do_something_else(250)
end I have no objection with the following, even though the repetition is not in the else. Assume that the predicate order can not be changed. case
when some_test? then :ignore
when some_sub_case? then :critical
when most_other_cases? then :ignore
else :normal_priority
end In short: I'm not convinced that the best config is distinguishing the |
Hmm, I think I like this idea better, although I'd probably name the config "IgnoreTrivial/LiteralBranches". It's true that if a branch is just a literal probably the duplication is not that big of a deal. |
@marcandre @bbatsov thanks for the input! I think there is more to be done to this cop to make it more useful for a wider variety of cases.
If there's no objection I'll update the cop to allow for all three configurations. |
Sorry I wasn't clear. My first example => offense. Second example => ok. The duplication checks the branch bodies, not the conditions. So my second example is a case that might not be easy / clearer to reorder the branches (complex conditions) but where branch bodies are basic literals. I see no harm in duplicated basic literals bodies.
I don't, and I have a problem with duplicated non basic body branches ( |
I also don't think that duplicated literal branches are big problem.
I can see some value there, but it's probably going to be an overkill. What I like about the literal branches idea is that it's simple, solves the original problem and doesn't require specialn handling for |
Thanks, I think I mixed up the condition and body in my previous message. I'll implement Are we still ok with an offense being registered here:
|
Yes, and even if |
9e694b6
to
3b64804
Compare
I've rewritten my changes to implement |
AllowCaseElseDuplication
option to Lint/DuplicateBranch
.IgnoreLiteralBranches
option to Lint/DuplicateBranch
.
3b64804
to
adc25e9
Compare
adc25e9
to
a941451
Compare
return false if branch.basic_literal? | ||
return true if branch.xstr_type? | ||
|
||
branch.each_descendant.any? do |node| |
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'm not 100% sure, but I would exclude hash literals however simple they are (same as arrays).
Are you allowing constants? I think we should.
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 added IgnoreConstantBranches
to ignore constants now.
I think it's more consistent to let IgnoreLiteralBranches
ignore all literals including hashes and arrays. @bbatsov what do you think?
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 agree. It's uncommon to have calculations done in literals, so I'd stick with a simple and consistent definition.
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 really don't think there should be a separate setting for constants.
Thinking about it more, in my mind it's actually not about being a literal but an already simple and well-defined object. As such a variable (not allowed currently, right?) / constant / integer / symbol are all acceptable. These involve no processing and no memory allocation. There's no good reason to factorize them, i.e. to store them / label them in one place before using them in multiple places. I'm not sure I have a name for this though. Atom? Interned? Simple values?
Just a last example, before you decide to keep hashes and arrays:
case
when method_a
{ hello: 'world',
:does_it? => 'make sense',
to: :accept_this,
}
when method_b
{ hi: 'world',
:does_it? => 'make sense',
to: :accept_this,
}
when method_c then
{ hello: 'world',
:does_it? => 'make sense',
to: :accept_this,
}
end
Compare this to:
case
when method_a then A
when method_b then B
when method_c then C
else B
end
Same with A/B/C/B
being variables.
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.
Yeah, I totally get that those scenarios would be possible, but they are not very common and anything we define as "trivial" or whatever would carry some cognitive overhead as people have to understand our definition of trivial. I'd rather go with something that leaves no room for interpretation and solves the problem more or less. Plus, we're not changing the defaults. :-)
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.
Agree with @bbatsov. The initial idea here was just to make this less restrictive based on standard's issue, which I thought was valid. I've iterated on this three times now but I think we're getting into nitpicking so I'd rather scope hammer this back to the original goal.
…Lint/DuplicateBranch`.
IgnoreLiteralBranches
option to Lint/DuplicateBranch
.IgnoreLiteralBranches
and IgnoreConstantBranches
options to Lint/DuplicateBranch
Thanks for tackling this! 🙇♂️ |
I noticed that the standard gem disabled
Lint/DuplicateBranch
and the straw that broke the camel's back for them was that this kind of case layout registered an offense:While it's true that the
medium
case and theelse
case are duplicates, I'd argue that this duplication is beneficial because it clearly outlines what possible values are expected.Introduced a new configuration,
IgnoreLiteralBranches
, which when true will not consider any branches that just return a basic literal value, or a composite literal (array, hash, etc.) that only contains basic literals (see test for examples). As well, addedIgnoreConstantBranches
to allow branches that just return a constant to be ignored.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.