refactor start script and add option for custom coverage result path #158
Conversation
repomaa
force-pushed
the
topic/custom_coverage_dir
branch
2 times, most recently
from
69d2a41 to
9d9c7ec
Compare
|
|
||
| COVERAGE_FILE = "coverage/.resultset.json".freeze |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure, no problem. Using option parser is maybe premature at this point. I'll update the pr.
repomaa
force-pushed
the
topic/custom_coverage_dir
branch
2 times, most recently
from
16849d1 to
6ebbf50
Compare
repomaa
changed the title
| $stderr.puts "Error encountered while parsing #{COVERAGE_FILE}: #{e}" | ||
| exit(1) | ||
| end | ||
| abort "Coverage results not found" unless File.exist?(COVERAGE_FILE) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
alright. I'd argue that abort is similar to puts and shouldn't be wrapped in parens, so i'll remove them in L18
repomaa
force-pushed
the
topic/custom_coverage_dir
branch
from
4c6e92d to
cb6a305
Compare
Fixes #143