metricfu / metric_fu

A fist full of code metrics
http://metricfu.github.com/metric_fu
MIT License
624 stars 96 forks source link

Add line numbers to reek output. #255

Closed ggallen closed 9 years ago

ggallen commented 9 years ago

Here's a version that includes the fixed tests.

MGotink commented 9 years ago

Thanks so far. Could you add some tests to verify that the line numbers are set correctly? It would be nice if we could add the line numbers to the report too, I'm not sure yet how to do this when having multiple line numbers, maybe use the first line number?

bf4 commented 9 years ago

bump. what do we want to do with what we have so far? What's the minimal amount to get merged?

ggallen commented 9 years ago

Sorry, I got pulled away but have not forgotten this.

I will comment the regexp as requested and look at adding the line number tests.

MGotink, I'm not sure what you mean by "add the line numbers to the report"....

ggallen commented 9 years ago

OK, I think I've updated the pull request. I wrestled with git for a while trying to get a single commit, but struck out there and I'm tired of trying right now.

Is there anything else you need from me?

MGotink commented 9 years ago

Sorry for the late reply. @ggallen, I meant adding the line numbers to the HTML reports. At the moment only the file is displayed.

ggallen commented 9 years ago

I've never looked at the HTML reports. Is this something you want before accepting these changes?

MGotink commented 9 years ago

Not necessarily, it can always be added later.

ggallen commented 9 years ago

Bump.

I'd like to wrap this up. If there's more you need or want from me please let me know.

bf4 commented 9 years ago

@MGotink I haven't tested this myself... please merge if you're happy with it and I'll push a release.

MGotink commented 9 years ago

Sorry, been busy with other stuff. It runs fine when metric_fu data already exists. However I get an empty reek report when I run metric_fu on itself, when no previous data is present: bundle exec ruby -Ilib bin/metric_fu

Yaml output:

:reek:
  :matches:
  - :file_path: lib/metric_fu.rb
    :code_smells: []
  - :file_path: lib/metric_fu/cli/client.rb
    :code_smells: []
  - :file_path: lib/metric_fu/cli/helper.rb
    :code_smells: []
  - :file_path: lib/metric_fu/cli/parser.rb
    :code_smells: []
...

@ggallen, can you have a look at it?

ggallen commented 9 years ago

I will take a look at it.

But what do you expect to see in this case?

MGotink commented 9 years ago

Something like

:reek:
  :matches:
  - :file_path: lib/metric_fu.rb
    :code_smells:
    - :method: MetricFu#current_time
      :message: doesn't depend on instance state
      :type: UtilityFunction
    - :method: MetricFu#run
      :message: doesn't depend on instance state
      :type: UtilityFunction
    - :method: MetricFu#run_only
      :message: contains iterators nested 2 deep
      :type: NestedIterators
    - :method: MetricFu#run_only
      :message: has approx 9 statements
      :type: TooManyStatements
  - :file_path: lib/metric_fu/cli/client.rb
    :code_smells:
    - :method: MetricFu::Cli::Client
      :message: has no descriptive comment
      :type: IrresponsibleModule
MGotink commented 9 years ago

I just took a look at it.

For reek > 1.1 the leading spaces of the output are stripped in ReekGenerator#massage_for_reek_12, so the regex in ReekGenerator#analyze doesn't split the result. Changing the gsub regex in

@matches = @output.chomp.split("\n\n").map { |m| m.gsub(/\n\s+\[/, "SPLIT_ME_HERE_PLEASE[").split("SPLIT_ME_HERE_PLEASE") }

from /\n\s+\[/ to /\n\s*\[/ fixes the issue.

Can you update the code with this change and add a spec for the newer format? Example output (without line numbers) can be found in the generator_spec.

bf4 commented 9 years ago

can we maybe just use reek directly?

MGotink commented 9 years ago

That would at least make processing the output a lot easier. But maybe we should wrap up this PR first, since it's almost done?

It seems to be pretty straightforward to do: https://github.com/troessner/reek/blob/develop/docs/API.md I might give it a try soon.

ggallen commented 9 years ago

Sorry @MGotink, I got sidetracked and wasn't able to get back to this. Did you just roll this change into #258? If so, thanks for your help....

MGotink commented 9 years ago

Yeah, your commits are included in that PR.