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

refactor start script and add option for custom coverage result path #158

Merged
merged 1 commit into from
Nov 30, 2016
Merged

refactor start script and add option for custom coverage result path #158

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
31 changes: 14 additions & 17 deletions bin/codeclimate-test-reporter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,20 @@

require "codeclimate-test-reporter"

COVERAGE_FILE = "coverage/.resultset.json".freeze
repo_token = ENV["CODECLIMATE_REPO_TOKEN"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I do like the improvements in this PR and am happy to continue reviewing it as-is, but before I do so, what would you think of solving this issue with a smaller, and perhaps even more flexible change of:

COVERAGE_FILE = ARGV.first || "coverage/.resultset.json".freeze

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem. Using option parser is maybe premature at this point. I'll update the pr.

if repo_token.nil? || repo_token.empty?
STDERR.puts "Cannot post results: environment variable CODECLIMATE_REPO_TOKEN must be set."
exit
end

COVERAGE_FILE = ARGV.first || "coverage/.resultset.json"

if (repo_token = ENV["CODECLIMATE_REPO_TOKEN"]) && !repo_token.empty?
if File.exist?(COVERAGE_FILE)
begin
results = JSON.parse(File.read(COVERAGE_FILE))
rescue JSON::ParserError => e
$stderr.puts "Error encountered while parsing #{COVERAGE_FILE}: #{e}"
exit(1)
end
abort "Coverage results not found" unless File.exist?(COVERAGE_FILE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you either user parens here, or don't use them at L18?

Our in-house style is to use parens except in special cases (e.g. puts). I think I'd consider abort a special case (so no parens needed), but since it's not clear in our style guide, you can decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright. I'd argue that abort is similar to puts and shouldn't be wrapped in parens, so i'll remove them in L18


CodeClimate::TestReporter.run(results)
else
$stderr.puts "Coverage results not found"
exit(1)
end
else
$stderr.puts "Cannot post results: environment variable CODECLIMATE_REPO_TOKEN must be set."
exit(0)
begin
results = JSON.parse(File.read(COVERAGE_FILE))
rescue JSON::ParserError => e
abort "Error encountered while parsing #{COVERAGE_FILE}: #{e}"
end

CodeClimate::TestReporter.run(results)