Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Run churn metric as library #182

Merged
merged 1 commit into from
Dec 7, 2013
Merged

Run churn metric as library #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions .metrics
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@
# cane.no_doc = 'y'
# cane.no_readme = 'y'
# end
#
# config.configure_metric(:churn) do |churn|
# churn.enabled = true
# churn.ignore_files = 'HISTORY.md, TODO.md'
# churn.start_date = '6 months ago'
# end
#
# Or, to configure a group of metrics...
# config.configure_metrics.each do |metric|
Expand Down
1 change: 1 addition & 0 deletions HISTORY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ As such, a _Feature_ would map to either major or minor. A _bug fix_ to a patch.
### Master [changes](https://github.com/metricfu/metric_fu/compare/v4.6.0...master)

* Features
* Move to using churn library and allowing all churn options to be passed through to churn library. (Dan Mayer, #182)
* Fixes
* Force gemspec to use utf-8 encoding when importing the AUTHORS file (Paul Swagerty, #183)
* Misc
Expand Down
58 changes: 16 additions & 42 deletions lib/metric_fu/metrics/churn/churn.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,61 +6,35 @@ def self.metric
:churn
end

###
# options available are what can be passed to churn_calculator
# https://github.com/danmayer/churn#library-options
###
def emit
@output = generate_churn_metrics
@output = run(options)
end

def analyze
if @output.nil? || @output.match(/Churning requires.*git/)
if @output.nil? || @output.size.zero?
Copy link
Member

Choose a reason for hiding this comment

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

See below re: output of run always being a hash

@churn = {:churn => {}}
else
@churn = YAML::load(@output)
@churn = @output
end
@churn
end

# ensure hash only has the :churn key
def to_h
{:churn => @churn[:churn]}
Copy link
Member

Choose a reason for hiding this comment

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

Kind of silly, no? @churn.has_key?(:churn)

Copy link
Member

Choose a reason for hiding this comment

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

Also, not part of this PR... just thoughts while reading the code.

Copy link
Author

Choose a reason for hiding this comment

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

So this because the churn output can actually have other keys. At least in the past when churn had something like

{
:churn => {...},
:something => {...},
:blah => {..}
}

That would cause parts of metric_fu to blow up. IF having other keys in the results no longer matters this could be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Oh.. worth a comment or link or something I guess

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if it still hold true and don't really have anywhere to link to. I could remove it and see if things blow up again.

end

private

def generate_churn_metrics
output = churn_code
ensure_output_is_valid_yaml(output)
end

def churn_code
run!(build_churn_options)
end

def ensure_output_is_valid_yaml(output)
yaml_start = output.index("---")
if yaml_start
output[yaml_start...output.length]
else
nil
end
end

def build_churn_options
opts = ["--yaml"]
churn_options.each do |churn_option, command_flag|
if has_option?(churn_option)
opts << "#{command_flag}=#{options[churn_option]}"
end
end
opts.join(" ")
end

def has_option?(churn_option)
options.include?(churn_option)
end
def churn_options
{
:minimum_churn_count => '--minimum_churn_count',
:start_date => '--start_date'
}

# @param args [Hash] churn metric run options
# @return [Hash] churn results
# @example {something}
def run(args)
Copy link
Member

Choose a reason for hiding this comment

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

# @param args [Hash] churn metric run options
# @return [Hash] churn results
# @example {something}

# @note passing in false to report will return a hash
# instead of the default String
::Churn::ChurnCalculator.new(args).report(false)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps return an empty hash if the calculator returns nil so we don't need the nil check above?

Copy link
Author

Choose a reason for hiding this comment

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

I can change how churn returns for a future version but need to check for current version.

Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss sometime, after this PR... My original thought was that the actual command for running the metric should be in MetricChurn (but this isn't fully-baked, right now.. except for run_external)

end

end
Expand Down
8 changes: 7 additions & 1 deletion lib/metric_fu/metrics/churn/init.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ def name
end

def default_run_options
{ :start_date => %q("1 year ago"), :minimum_churn_count => 10}
{
:start_date => %q("1 year ago"),
Copy link
Member

Choose a reason for hiding this comment

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

Is this any date parseable by Chronic?

Copy link
Author

Choose a reason for hiding this comment

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

Yes that is correct. What kind of date parsing is metric_fu mostly settled on. I was considering a few fixes and moving churn to 1.0.0 and as part of that dropping chronic for a simpler more standardized date format. Mostly to reduce dependancies and complexity. If you think metric_fu is moving towards chronic like date usage, I am happy to keep it. If not it would be awesome to move towards something standard like unix date

date
Tue Dec 3 22:33:30 EST 2013

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Whatever you decide is fine. I can see both sides.

MetricFu doesn't use Chronic at this time and I haven't found a use-case for it yet. I really like Chronic.

:minimum_churn_count => 10,
:ignore_files => [],
:data_directory => MetricFu::Io::FileSystem.scratch_directory(name)
}
end

def has_graph?
Expand All @@ -18,6 +23,7 @@ def enable
end

def activate
activate_library('churn/churn_calculator')
super
end

Expand Down
2 changes: 1 addition & 1 deletion spec/metric_fu/configuration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ class DummyTemplate;end
it 'should set @churn to {}' do
load_metric 'churn'
expect(MetricFu::Metric.get_metric(:churn).run_options).to eq(
{ :start_date => %q("1 year ago"), :minimum_churn_count => 10}
{ :start_date => %q("1 year ago"), :minimum_churn_count => 10, :ignore_files=>[], :data_directory=> MetricFu::Io::FileSystem.scratch_directory('churn')}
)
end

Expand Down
34 changes: 5 additions & 29 deletions spec/metric_fu/metrics/churn/churn_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,43 +4,19 @@
describe MetricFu::ChurnGenerator do

# TODO extract yaml
let(:churn_yaml) { "--- \n:churn: \n :changed_files: \n - spec/graphs/flog_grapher_spec.rb\n - spec/base/graph_spec.rb\n - lib/templates/awesome/layout.html.erb\n - lib/graphs/rcov_grapher.rb\n - lib/base/base_template.rb\n - spec/graphs/grapher_spec.rb\n - lib/templates/awesome/flog.html.erb\n - lib/templates/awesome/flay.html.erb\n - lib/graphs/roodi_grapher.rb\n - lib/graphs/reek_grapher.rb\n - HISTORY\n - spec/graphs/roodi_grapher_spec.rb\n - lib/generators/rcov.rb\n - spec/graphs/engines/gchart_spec.rb\n - spec/graphs/rcov_grapher_spec.rb\n - lib/templates/javascripts/excanvas.js\n - lib/templates/javascripts/bluff-min.js\n - spec/graphs/reek_grapher_spec.rb\n" }
let(:churn_hash) { YAML::load("--- \n:churn: \n :changed_files: \n - spec/graphs/flog_grapher_spec.rb\n - spec/base/graph_spec.rb\n - lib/templates/awesome/layout.html.erb\n - lib/graphs/rcov_grapher.rb\n - lib/base/base_template.rb\n - spec/graphs/grapher_spec.rb\n - lib/templates/awesome/flog.html.erb\n - lib/templates/awesome/flay.html.erb\n - lib/graphs/roodi_grapher.rb\n - lib/graphs/reek_grapher.rb\n - HISTORY\n - spec/graphs/roodi_grapher_spec.rb\n - lib/generators/rcov.rb\n - spec/graphs/engines/gchart_spec.rb\n - spec/graphs/rcov_grapher_spec.rb\n - lib/templates/javascripts/excanvas.js\n - lib/templates/javascripts/bluff-min.js\n - spec/graphs/reek_grapher_spec.rb\n") }
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to put this yaml in a file fixture in spec/fixtures


let(:config_setup) {
ENV['CC_BUILD_ARTIFACTS'] = nil
MetricFu.configure.reset
}

describe "new method" do
before :each do
config_setup
end

it "initializes with yaml option" do
churn = MetricFu::ChurnGenerator.new
churn.send(:build_churn_options).should == "--yaml"
end

it "initializes with given minimum_churn_count option" do
churn = MetricFu::ChurnGenerator.new( { :minimum_churn_count => 5 })
churn.send(:build_churn_options).should == "--yaml --minimum_churn_count=5"
end
end


describe "analyze method" do
before :each do
config_setup
@changes = {"lib/generators/flog.rb" => 2, "lib/metric_fu.rb" => 3}
end

it "should be empty on error text" do
churn = MetricFu::ChurnGenerator.new
churn.instance_variable_set(:@output, "Churning requires a subversion or git repo")
result = churn.analyze
result.should == {:churn => {}}
end

it "should be empty on error no output captured" do
churn = MetricFu::ChurnGenerator.new
churn.instance_variable_set(:@output, nil)
Expand All @@ -50,7 +26,7 @@

it "should return yaml results" do
churn = MetricFu::ChurnGenerator.new
churn.instance_variable_set(:@output, churn_yaml)
churn.instance_variable_set(:@output, churn_hash)
result = churn.analyze
result.should == {:churn => {:changed_files => ["spec/graphs/flog_grapher_spec.rb", "spec/base/graph_spec.rb", "lib/templates/awesome/layout.html.erb", "lib/graphs/rcov_grapher.rb", "lib/base/base_template.rb", "spec/graphs/grapher_spec.rb", "lib/templates/awesome/flog.html.erb", "lib/templates/awesome/flay.html.erb", "lib/graphs/roodi_grapher.rb", "lib/graphs/reek_grapher.rb", "HISTORY", "spec/graphs/roodi_grapher_spec.rb", "lib/generators/rcov.rb", "spec/graphs/engines/gchart_spec.rb", "spec/graphs/rcov_grapher_spec.rb", "lib/templates/javascripts/excanvas.js", "lib/templates/javascripts/bluff-min.js", "spec/graphs/reek_grapher_spec.rb"]}}
end
Expand Down Expand Up @@ -78,13 +54,13 @@
end

it "returns churn output" do
@churn.stub(:churn_code).and_return(" master\n#{churn_yaml}")
@churn.stub(:run).and_return(churn_hash)
result = @churn.emit
result.should == churn_yaml
result.should == churn_hash
end

it "returns nil, when churn result is not yaml" do
@churn.stub(:churn_code).and_return(" master\n")
@churn.stub(:run).and_return(nil)
result = @churn.emit
result.should be nil
end
Expand Down
12 changes: 10 additions & 2 deletions spec/metric_fu/run_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,18 @@ def data_directory
# limited set, so we can test the basic functionality
# without significantly slowing down the specs.
MetricFu.configuration.configure_metrics do |metric|
if metric.name == :churn
if metric.name == :reek
Copy link
Member

Choose a reason for hiding this comment

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

ha ha ha!

metric.enable
metric.activated = true
metric.should_receive(:run_external).and_return('')
# so this doesn't seem to always be true
# @bf4 adding that line you thought I didn't need made tests pass for me
# but they failed on one of the travis builds.
# swapping this line all seem to pass every run
# I don't understand what is happening in run_external well enough
# need some help debugging
# but tests seem more stable without it.
# metric.should_receive(:run_external).and_return('')
metric.stub(:run_external).and_return('')
else
metric.enabled = false
end
Expand Down