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.

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

Adding support for running metrics against a particular git hash #188

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
9 changes: 9 additions & 0 deletions lib/metric_fu/cli/parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def configure_options(p)

add_format_option(p)
add_output_option(p)
add_git_option(p)

p.banner = @banner unless @banner.nil?
p.on_tail("-h", "--help", "Show this message") {puts p ; success!}
Expand Down Expand Up @@ -76,6 +77,14 @@ def add_general_option(p, o)
end
end

def add_git_option(p)
p.on('--githash COMMIT_HASH',
'Specify the git hash run metric_fu against') do |o|
@result[:githash] ||= ''
@result[:githash] << o
end
end

def add_output_option(p)
p.on("--out FILE|DIR",
"Specify the file or directory to use for output",
Expand Down
4 changes: 2 additions & 2 deletions lib/metric_fu/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ def metric_manually_configured?(metric)
end

# TODO: Reconsider method name/behavior, as it really adds a formatter
def configure_formatter(format, output = nil)
@formatters << MetricFu::Formatter.class_for(format).new(output: output)
def configure_formatter(format, output = nil, filename = nil)
@formatters << MetricFu::Formatter.class_for(format).new(output: output, filename: filename)
end

# @return [Array<Symbol>] names of enabled metrics with graphs
Expand Down
8 changes: 7 additions & 1 deletion lib/metric_fu/formatter/html.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def finish
mf_debug "** SAVING REPORT DATA OUTPUT TO #{MetricFu::Io::FileSystem.directory('data_directory')}"
# TODO: Allow customizing output filenames
MetricFu::Formatter::YAML.new(
output: MetricFu.run_path.join("#{MetricFu::Io::FileSystem.directory('data_directory')}/#{Time.now.strftime("%Y%m%d")}.yml")
output: output_yml
).finish

mf_debug "** SAVING TEMPLATIZED REPORT"
Expand All @@ -42,6 +42,12 @@ def output_directory
@output ||= dir_for(@options[:output]) || MetricFu.run_path.join(MetricFu::Io::FileSystem.directory('output_directory'))
end

def output_yml
filename_prefix = @options[:filename] || Time.now.strftime("%Y%m%d")
Copy link
Member

Choose a reason for hiding this comment

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

If you feel ambitious, this would be a good place to factor out the notion of "Report Time" (what time in the code the reported metrics were run).

Copy link
Member

Choose a reason for hiding this comment

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

This also seems mis-named. It's the filename, not the prefix.


MetricFu.run_path.join("#{MetricFu::Io::FileSystem.directory('data_directory')}/#{filename_prefix}.yml")
end

# Instantiates a new template class based on the configuration set
# in MetricFu::Configuration, or through the MetricFu.config block
# in your rake file (defaults to the included AwesomeTemplate),
Expand Down
10 changes: 8 additions & 2 deletions lib/metric_fu/metrics/graph.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,11 @@ def generate_graphs_for_file(metric_file)
date_parts = year_month_day_from_filename(metric_file)
metrics = MetricFu::Utility.load_yaml(metric_file)

build_graph(metrics, "#{date_parts[:m]}/#{date_parts[:d]}")
# Determine the name of the graph files based on the original metric_file name
graph_filename = metric_file
graph_filename = "#{date_parts[:m]}/#{date_parts[:d]}" if date_parts

build_graph(metrics, graph_filename)
rescue NameError => e
mf_log "#{e.message} called in MetricFu::Graph.generate with #{metric_file}"
end
Expand All @@ -66,7 +70,9 @@ def graph!
end

def year_month_day_from_filename(path_to_file_with_date)
date = path_to_file_with_date.match(/\/(\d+).yml$/)[1]
date = path_to_file_with_date.match(/\/(\d+).yml$/)
return nil unless date
date = date[1]
{:y => date[0..3].to_i, :m => date[4..5].to_i, :d => date[6..7].to_i}
end
end
Expand Down
38 changes: 36 additions & 2 deletions lib/metric_fu/run.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'git'
MetricFu.configure
module MetricFu
class Run
Expand All @@ -7,6 +8,7 @@ def initialize
def run(options={})
configure_run(options)
measure
reset_git(options) if git_configured? options
display_results if options[:open]
end

Expand All @@ -28,6 +30,7 @@ def display_results
private
def configure_run(options)
disable_metrics(options)
configure_git(options)
configure_formatters(options)
end
def disable_metrics(options)
Expand Down Expand Up @@ -55,20 +58,51 @@ def configure_metric(metric, metric_options)
end
end
def configure_formatters(options)
filename = options[:githash] # Set the filename for the output; nil is a valid value
Copy link
Member

Choose a reason for hiding this comment

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

That's a little misleading, no? Why not just do

metric_filename = options.fetch(:githash) { filename_timestamp(Time.now) }

def filename_timestamp(time)
  time.strftime("%Y%m%d") 
end


# Configure from command line if any.
if options[:format]
MetricFu.configuration.formatters.clear # Command-line format takes precedence.
Array(options[:format]).each do |format, o|
MetricFu.configuration.configure_formatter(format, o)
MetricFu.configuration.configure_formatter(format, o, filename)
Copy link
Member

Choose a reason for hiding this comment

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

no need to pass the filename if we combine its logic with the default filename (filenamed_timestamp)

end
end
# If no formatters specified, use defaults.
if MetricFu.configuration.formatters.empty?
Array(MetricFu::Formatter::DEFAULT).each do |format, o|
MetricFu.configuration.configure_formatter(format, o)
MetricFu.configuration.configure_formatter(format, o, filename)
end
end
end
def configure_git(options)
return unless git_configured? options

Copy link
Member

Choose a reason for hiding this comment

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

Here you'd want to use a version-control adapter that can require and use arbitrary library code. See comment below re: gem dependency

begin
@git ||= Git.init
@orig_branch ||= @git.current_branch

mf_log "CHECKING OUT '#{options[:githash]}'"
@git.checkout(options[:githash])
rescue Git::GitExecuteError => e
mf_log "Unable to checkout githash: #{options[:githash]}"
raise "Unable to checkout githash: #{options[:githash]}: #{e}"
end
end
def reset_git(options)
return unless git_configured? options

begin
@git ||= Git.init
mf_log "CHECKING OUT ORIGINAL BRANCH '#{@orig_branch}'"
@git.checkout(@orig_branch)
rescue Git::GitExecuteError => e
mf_log "Unable to reset git status to branch '#{@orig_branch}'"
raise "Unable to reset git status to branch '#{@orig_branch}'"
end
end
def git_configured?(options)
options.size > 0 && options.has_key?(:githash)
end
def reporter
MetricFu::Reporter.new(MetricFu.configuration.formatters)
end
Expand Down
1 change: 1 addition & 0 deletions lib/metric_fu/utility.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require 'yaml'
require 'fileutils'
require 'git'
Copy link
Member

Choose a reason for hiding this comment

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

remove per other comments

module MetricFu
module Utility
module_function
Expand Down
6 changes: 4 additions & 2 deletions metric_fu.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ Gem::Specification.new do |s|
s.authors = File.readlines(author_file, :encoding => Encoding::UTF_8).map(&:strip)

# used with gem i metric_fu -P HighSecurity
s.cert_chain = ['certs/bf4.pem']
s.signing_key = File.expand_path("~/.ssh/gem-private_key.pem") if $0 =~ /gem\z/
#s.cert_chain = ['certs/bf4.pem']
#s.signing_key = File.expand_path("~/.ssh/gem-private_key.pem") if $0 =~ /gem\z/
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing you didn't mean to include this in the PR, but I understand why you made it :)


s.rubyforge_project = 'metric_fu'
s.license = 'MIT'
Expand Down Expand Up @@ -55,6 +55,8 @@ Gem::Specification.new do |s|
s.add_runtime_dependency 'coderay'
# to_json support
s.add_runtime_dependency 'multi_json'
# git support
s.add_runtime_dependency 'git'
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 rather not add a hard dependency on the git gem or other vcs gems like grit etc.. Perhaps we can look at how churn or turbulence do it, or with the git hash option do something like

def select_git_hash
   require 'git'
   # do stuff
rescue LoadError
  STDERR.puts "Cannot select git hash; please install git gem"
  exit(1)
end


# temporary filesystem to act on
s.add_development_dependency 'test-construct'
Expand Down
6 changes: 6 additions & 0 deletions spec/cli/helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,12 @@
end
end

context 'given a single git hash/tag' do
it 'sets the git flag' do
helper.process_options(['--githash', 'testhash'])[:githash].should eq 'testhash'
end
end

end

end
7 changes: 7 additions & 0 deletions spec/metric_fu/formatter/html_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ def directory(name)
}.to create_file("#{directory('data_directory')}/#{Time.now.strftime("%Y%m%d")}.yml")
end

it 'creates a data yaml file based on specified filename' do
fn = 'testfilename'
expect {
MetricFu::Formatter::HTML.new(filename: fn).finish
}.to create_file("#{directory('data_directory')}/#{fn}.yml")
end

it "creates a report index html file" do
expect {
MetricFu::Formatter::HTML.new.finish
Expand Down
25 changes: 25 additions & 0 deletions spec/metric_fu/run_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,31 @@ def data_directory
expect { metric_fu }.to create_file("#{output_directory}/index.html")
end

context 'with a git-hash specified' do
let(:githash) { 'testhash' }

before do
@branch = 'original_branch'

# Stub out the git checkout method to not actually checkout a branch, but keep track of the state
Git::Base.any_instance.stub(:checkout) do |branch|
@branch = branch
end

Git::Base.any_instance.stub(:current_branch) { @branch }
end

it "creates a git-hash yaml file" do
expect { metric_fu "--githash #{githash} --no-open" }.to create_file("#{data_directory}/#{githash}.yml")
end

it 'resets context back to the original git state' do
metric_fu "--githash #{githash} --no-open"

expect(@branch).to eq 'original_branch'
end
end

context "with configured formatter" do
it "outputs using configured formatter" do
expect {
Expand Down