[Feature] Add support for JSON output #900
Conversation
|
Looks good. I think I'd prefer to follow SwiftLint's pattern of |
|
Thanks for the review @nicklockwood, can you have a look now? 🙌 |
Sources/CommandLine.swift
Outdated
| @@ -198,6 +198,8 @@ func printHelp(as type: CLI.OutputType) { | |||
| --lenient Suppress errors for unformatted code in --lint mode | |||
| --verbose Display detailed formatting output and warnings/errors | |||
| --quiet Disables non-critical output messages and warnings | |||
| --reporter Defines a custom reporter (only `json` is supported) | |||
| --reporter-output Reporter output file (defaults to `output.<reporter>`) | |||
There was a problem hiding this comment.
The --reporter-output parameter seems redundant - we can just re-use the existing --output parameter.
Also, the output shouldn't default to a hard-coded filename if no output file is specified, it should just write the json to stdout.
There was a problem hiding this comment.
Isn't --output the output of the formatted file? reusing it here would be a bit confusing 🤔
--output Output path for formatted file(s) (defaults to input path)
What about having just a --report parameter, and determine the type from the provided file extension?
There was a problem hiding this comment.
Replaced reporter and reporter-output with single report parameter (from which also the type can be inferred in the future.
Please let me know whether it is fine 🙌
There was a problem hiding this comment.
The problem with that is that if you don't specify an --output file then there's no way for it to infer what format should be sent to stdout.
There was a problem hiding this comment.
The --report parameters tales in input the report file, not the type, so you can use it as:
swiftformat <whatever option> --report report.json
So when printing the json report the rest of the output and the parameters are not affected.
What do you think?
There was a problem hiding this comment.
@nicklockwood did you had time to have a look at this?
Not urgent at all, just wanted to be sure you didn't miss my message 🙌
There was a problem hiding this comment.
Hmm, I was under the impression that reporting was only applicable when running in --lint mode, in which case --output is unused, so having --report take a separate path seemed redundant. But I now realize that you're intending for report to work even when formatting? In which case I agree a separate --report path makes sense.
There was a problem hiding this comment.
For my use case I would use it only for linting, but I think having a separate parameter may be more generic and cover more use cases 🙌
Sources/CommandLine.swift
Outdated
| guard reporter == "json" else { | ||
| throw FormatError.options("Unsupported reporter: \(reporter).") | ||
| } | ||
| let outputFile = args["reporter-output"] ?? "output.\(reporter)" |
There was a problem hiding this comment.
Instead of inferring output file from reporter, we could infer the reporter from the output file, so if the user specifies an output file with a json extension it could preselect json as the exporter type.
Sources/JSONReporter.swift
Outdated
|
|
||
| public func encode(to encoder: Encoder) throws { | ||
| var container = encoder.container(keyedBy: CodingKeys.self) | ||
| try container.encode(self.filePath ?? "", forKey: .file) |
There was a problem hiding this comment.
Does it make sense to write a blank string if the filePath is nil rather than just omitting the key?
Sources/JSONReporter.swift
Outdated
| case file | ||
| case line | ||
| case reason | ||
| case rule_id |
There was a problem hiding this comment.
I'd prefer to us camelCase here, in accordance with Swift naming conventions:
case ruleID = "rule_id"
Fix for #890
The JSON format is inspired by the format of SwiftLint JSON reporter