Fix IfUnlessModifier's co-operation with LineLength in edge cases #7507
Conversation
jonas054
force-pushed
the
fix_IfUnlessModifier_vs_LineLength_edge_cases
branch
from
db69f25
to
b75b608
Compare
jonas054
force-pushed
the
fix_IfUnlessModifier_vs_LineLength_edge_cases
branch
from
b75b608
to
1b39d3e
Compare
|
Rebased and resolved conflicts. I would love some reviews. :) |
| module RuboCop | ||
| module Cop | ||
| # Help methods for determining if a line is too long. | ||
| class LineLengthHelp |
There was a problem hiding this comment.
I'd use a different name here, as we haven't used the SomethingHelp pattern anywhere else. I also noticed you haven't used this class as a mixin, even though it's in the mixin dir.
There was a problem hiding this comment.
There are actually a couple of mixin modules with names ending in Help: RangeHelp, StringHelp, and StringLiteralsHelp. Only the last one is in the mixin directory, but I think that's because it's the only one used from multiple departments. So since I can't think of a better name than Help, and since there is some precedent, I propose to keep the name.
The placement of a class in mixin is not ideal, I can see that. There is a utils namespace where we could put it, but I'm not sure if that's better. Maybe I can just discard the commit that changes the module into a class? Using composition instead of inheritance doesn't make a huge difference in this case.
There was a problem hiding this comment.
There are actually a couple of mixin modules with names ending in Help: RangeHelp, StringHelp, and StringLiteralsHelp. Only the last one is in the mixin directory, but I think that's because it's the only one used from multiple departments. So since I can't think of a better name than Help, and since there is some precedent, I propose to keep the name.
Ah, I totally forgot about those. Fair enough.
The placement of a class in mixin is not ideal, I can see that. There is a utils namespace where we could put it, but I'm not sure if that's better. Maybe I can just discard the commit that changes the module into a class? Using composition instead of inheritance doesn't make a huge difference in this case.
I'll leave this up to you. I don't have any strong preferences here. If it weren't for the delegation I'd say this was an util, but now it's hard to decide. OOP is way too complex. :-)
There was a problem hiding this comment.
I just took a look and it looks great. And for what it's worth I generally prefer composition over inheritance. ;)
…odifier Extract some code that can detect lines that are too long with all the special cases that are controlled through configuration parameters of the Metrics/LineLength cop. The code that is moved to the LineLengthHelp mixin can then be used in IfUnlessModifier so that it only reports lines on modifier form that are too long when the LineLength cop would report them.
jonas054
force-pushed
the
fix_IfUnlessModifier_vs_LineLength_edge_cases
branch
from
1b39d3e
to
bfdda17
Compare
|
@jonas054 thank you for this. ❤ |
|
@bbatsov I removed @koic as reviewer now that you and @jfelchner have both looked at it. I addressed your concerns about the class placed under |
|
Agreed. Thanks! |
There's a lot of complexity in how
Metrics/LineLengthhandles various exceptions regarding long lines. The best way forStyle/IfUnlessModifierto avoid false positives on long lines is to share code withMetrics/LineLengthand thus always have the same idea about which lines are too long.