WIP: cyclomatic complexity: indicate full method in location #9
Conversation
This is a horrible no good very bad take at fixing the issue with being unable to see method source in reports for cyclomatic complexity issues, but the alternative is a major pain, so I decided to at least try this first to confirm it really fixes the issue. So, the underlying reason that cyclomatic complexity violations repoorted by RuboCop weren't coming through with the full method def (I think) is because for complexity violations RuboCop reports the `location` of the violation as just the method definition line, not the whole method definition. Getting the full method location is pretty easy if you have the AST node. Unfortunately, RuboCop doesn't actually keep the AST `node` attached to the `Offense` instances: they get dropped when `Cop` instances generate their `Offense` instances. So by the time Rubocop's results come back to our code, there is no AST `node` for us to look at: just a string buffer of file contents & a location pointing at a method name. So this monkey patch basically just overrides the `Offense` creation on complexity cops to pass along the location as the whole method def instead of just the method def line. The alternative, to do it in our own code would, I think be: 1. Parse the string buffer ourself ourself to get an AST 2. Descend into AST to find node corresponding to the location RuboCop gave us 3. Map that node back to a location of the entire method.
| module RuboCop | ||
| module Cop | ||
| module MethodComplexity | ||
| def add_offense(node, loc, message = nil, severity = nil) |
There was a problem hiding this comment.
What do you think of adding a comment here about what/why this is? Similar to what you have in the commit message
There was a problem hiding this comment.
Yeah, thanks, I agree. Want to keep it shorter than the commit message explanation, and not sure how I want to word it yet. I'll ping you when I have something.
There was a problem hiding this comment.
FWIW: I'm a big fan of good commit messages and a team that goes to git blame immediately over leaving "wtf?" comments in the source. In this case a small comment and big commit message seems like an OK compromise though.
There was a problem hiding this comment.
Well, I've added a comment now, but it's not short. I'm not sure how much briefer it can be & still be good english without just cutting it down to "see commit message for why this is begin done."
There was a problem hiding this comment.
I don't feel strongly, I think this is fine as-is.
|
LGTM |
| # like complexity checkers, is just the method def line. This isn't very | ||
| # useful for checkers where the entire method body is relevant. Fetching | ||
| # this information from an `Offense` instance is difficult, since the | ||
| # original AST is no longer available. So it's easier to monkey-path |
There was a problem hiding this comment.
monkey-patch ⭐
I like the detail in this explanation. If you prefer brevity, what about something like:
Overwrite RuboCop to send entire offending method as offense, not just def line.
…l-method WIP: cyclomatic complexity: indicate full method in location
| # Monkey patching! Here be dragons. | ||
| module RuboCop | ||
| module Cop | ||
| module MethodComplexity |
There was a problem hiding this comment.
I'd define this as:
module RuboCop::Cop
MethodComplexity.module_eval do
# ...
end
end
Reason: If any of these module names change, it will fail fast.
There was a problem hiding this comment.
Or:
RuboCop::Cop::MethodComplexity.module_eval do
end
|
@wfleming @gordondiggs we should open an upstream ticket to check if this is something that would be valuable to Rubocop proper. It seems like it would be better to keep the whole node location info, and then if e.g. a text-based formatter just wants to print a single line, it can. |
I believe that this trello card is because RuboCop's behavior is to return only the method definition line for all complexity checkers.
The fix here is dirty, but the alternative seems kind of worse. Commit message on 00b63ed has more details on how RuboCop behaves, and why monkey patching seems better in this case.
Thoughts, @codeclimate/review?