(maint) Replace --report-* options with --format. #24
Conversation
This adds --format as a global option even though initially it may only be relevant to the `validate` and `test` subcommands.
|
/cc @scotje a couple of things about this:
|
lib/pdk/cli/util/option_parser.rb
Outdated
| module PDK | ||
| module CLI | ||
| module Util | ||
| class OptionParser |
There was a problem hiding this comment.
Note: I know the name OptionParser kind of sucks. Suggestions welcome :)
There was a problem hiding this comment.
There is a built-in OptionParser class so we probably shouldn't use that.
There was a problem hiding this comment.
Changed to OptionNormalizer
lib/pdk/cli/util/option_parser.rb
Outdated
| format, target = f, PDK::Report.default_target | ||
| end | ||
|
|
||
| reports << Report.new(target, format) |
There was a problem hiding this comment.
My plan here is to always work with an array of one or more reports, and we'll then just invoke write on however many we've got at the end of validation / testing. Each Report will deal with the specifics of writing its format to its target.
lib/pdk/report.rb
Outdated
| @@ -13,6 +13,10 @@ def self.default_format | |||
| 'junit' | |||
| end | |||
|
|
|||
| def self.default_target | |||
| 'stdout' # TODO: What's the default target? | |||
There was a problem hiding this comment.
Yes, everything should report to STDOUT unless otherwise specified.
lib/pdk/validators/metadata.rb
Outdated
| PDK.logger.info("Running #{cmd}") | ||
| output = PDK::CLI::Exec.execute(cmd) | ||
| report.write(output) if report | ||
| if reports | ||
| reports.each do |r| |
There was a problem hiding this comment.
All of this duplicated code across the 4 validators will go away after my other PR is merged and this fun stuff is merged into a base Validator class.
There was a problem hiding this comment.
So I'm wondering if it would be better for the components to just return a structure with like the exit-code, stdout, and stderr from the invoked command and then have the CLI part be responsible for passing that into the various formatters. Output formatting seems like it belongs in the interface layer of the application and then we won't have to pass the reports array in.
There was a problem hiding this comment.
Updated to do just that. I fixed up the exec method to create a CommandResult struct and return it. I'm not sure if an actual struct is the approach we'll want to ultimately take, but I suppose it does give us a consistent interface for command data (as opposed to just returning a plain old hash with {exit, stdout, stderr})
lib/pdk/cli/exec.rb
Outdated
| process.start | ||
|
|
||
| # wait indefinitely for process to exit... | ||
| process.wait | ||
|
|
||
| # get the exit code | ||
| process.exit_code #=> 0 | ||
| stdout = process.io.stdout.open.read |
There was a problem hiding this comment.
@scotje here's some fun with Tempfile. I wonder if I need to do and validation that it's actually open and I can read?
This commit updates the `validate` subcommand to use the new `format` option, which specifies one or more <format>:<target> pairs. Given multiple report formats and targets, the validators will create multiple reports, one for each.
DavidS
changed the title
The first of these commits adds --format as a global option even though initially it may only
be relevant to the validate and test subcommands.
The second commit adds a new
OptionParserUtil class and a method to parse the report formats and targets which were provided.The final commits update the
validatesubcommand to use the newformatoption, which specifies one or more format:target pairs.Given multiple report formats and targets, the validators will create multiple reports, one for each.