json output install #392
json output install #392
Conversation
roulpriya
requested review from
shahmishal,
cmcgee1024,
justice-adams-apple and
etcwilde
as code owners
roulpriya
force-pushed
the
json-output-install
branch
from
66d38b0 to
0a3c929
Compare
roulpriya
force-pushed
the
json-output-install
branch
from
0a3c929 to
6d83aef
Compare
| if let progressFile | ||
| { | ||
| try JsonFileProgressReporter(ctx, filePath: progressFile) | ||
| } else if ctx.format == .json { |
There was a problem hiding this comment.
question: Since we've been generally putting messages on stderr when json formatted output is requested, could the ConsoleProgressReporter do the same thing?
roulpriya
force-pushed
the
json-output-install
branch
from
9e1063a to
176e123
Compare
| @@ -312,16 +318,18 @@ struct Install: SwiftlyCommand { | |||
| } | |||
| } | |||
|
|
|||
| let animation: ProgressReporterProtocol = | |||
| let animation: ProgressReporterProtocol? = | |||
There was a problem hiding this comment.
suggestion: Now that the animation will always get a non-nil value in each case, this variable can become non-optional type. That should make the code below a bit simpler since it doesn't have to handle the optionality in every case.
There was a problem hiding this comment.
I have a condition that requires animation to be optional for the JSON format.
There was a problem hiding this comment.
Ah, I see now. Can the ctx.format == .json case create a ConsoleProgressReporter with stderr as the stream, and then the else case uses stdout as the stream?
There was a problem hiding this comment.
yes, that is doable.
| } else { | ||
| ConsoleProgressReporter(stream: stdoutStream, header: "Downloading \(version)") | ||
| ConsoleProgressReporter(stream: stderrStream, header: "Downloading \(version)") |
There was a problem hiding this comment.
issue (blocking): The default case should continue to report progress to stdout. The json output case would be the one to output to stderr, so that the progress wouldn't corrupt the json.
There was a problem hiding this comment.
Ohh, yes I missed this while earlier review.
roulpriya
force-pushed
the
json-output-install
branch
from
354bdd9 to
5d0f49f
Compare
Fixes Issue #376