Skip to content

WIP: cyclomatic complexity: indicate full method in location #9

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

Merged
merged 4 commits into from
Aug 18, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/cc/engine/rubocop.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require "json"
require "pathname"
require "rubocop/cop/method_complexity_patch"
require "rubocop"
require "cc/engine/category_parser"

Expand Down
18 changes: 18 additions & 0 deletions lib/rubocop/cop/method_complexity_patch.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Monkey patching! Here be dragons.
module RuboCop
module Cop
module MethodComplexity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or:

RuboCop::Cop::MethodComplexity.module_eval do
end

# RuboCop's default implementation of `add_offense` in `Cop` only gets the
# location of the keyword associated with the problem which, for things
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# this method on complexity checkers to send the location of the entire
# method to the created `Offense`.
def add_offense(node, loc, message = nil, severity = nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of adding a comment here about what/why this is? Similar to what you have in the commit message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly, I think this is fine as-is.

super(node, node.loc, message, severity)
end
end
end
end
40 changes: 40 additions & 0 deletions spec/cc/engine/rubocop_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,46 @@ def method
assert_equal location, result["location"]
end

it "includes complete method body for cyclomatic complexity issue" do
create_source_file("my_script", <<-EORUBY)
#!/usr/bin/env ruby

def method(a,b,c,d,e,f,g)
r = 1
if a
if !b
if c
if !d
if e
if !f
(1..g).each do |n|
r = (r * n) - n
end
end
end
end
end
end
end
r
end
EORUBY
output = run_engine
assert includes_check?(output, "Metrics/CyclomaticComplexity")

json = JSON.parse('[' + output.split("\u0000").join(',') + ']')

result = json.select { |i| i && i["check_name"] =~ /Metrics\/CyclomaticComplexity/ }.first
location = {
"path" => "my_script",
"positions" => {
"begin" => { "column"=>11, "line"=>3 },
"end" => { "column"=>14, "line"=>21 }
}
}
assert_equal location, result["location"]
end

def includes_check?(output, cop_name)
issues = output.split("\0").map { |x| JSON.parse(x) }

Expand Down