Implement support for automatically coloring output in DiagnosticsFormatter #1280
Conversation
|
@swift-ci please test |
|
Looks like tests are failing because I forgot to add |
|
@swift-ci please test |
|
Ah, the remaining macOS failure is because we've been trying not to introduce a Darwin dependency into the parts of swift-syntax that get linked into the compiler. |
|
I have a concrete suggestion. How about we move the TTY detection logic down into |
simba909
force-pushed
the
feature/automagically-colored-diagnostics
branch
from
24f60c9 to
81c8740
Compare
|
@DougGregor thank you for reviewing! I've moved the TTY detection logic and made the |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
| let annotation = ANSIAnnotation(color: .yellow) | ||
| return annotation.applied(to: message.message) | ||
| let color = ANSIAnnotation(color: .yellow) | ||
| let prefix = color.withTrait(.bold).applied(to: "warning: ") |
There was a problem hiding this comment.
Just to be clear: It’s intentional that errors have both the prefix and the the message in bold but warnings only have a bold prefix? I’m fine with either way but just want to make sure the current design is a conscious decision.
| return annotation.applied(to: message.message) | ||
| let color = ANSIAnnotation(color: .yellow) | ||
| let prefix = color.withTrait(.bold).applied(to: "warning: ") | ||
| return prefix + color.applied(to: message.message) | ||
| case .note: | ||
| return message.message |
There was a problem hiding this comment.
Should we also add a prefix for notes?
| static var isConnectedToTerminal: Bool { | ||
| return isTTY(stderr) | ||
| } |
There was a problem hiding this comment.
Since we are printing the errors to stdout (instead of stderr like swift-format), we should be checking whether stdout is connected to a terminal here. For example, stdout could be connected to a terminal while stderr is not if you do swift-parser-cli … 2> /tmp/errors.txt.
And, to be clear, we should probably name the method to mention that is checking stdout instead of stderr.
| static var isConnectedToTerminal: Bool { | |
| return isTTY(stderr) | |
| } | |
| static var isStdoutConnectedToTerminal: Bool { | |
| return isTTY(stdout) | |
| } |
| #if os(Android) | ||
| typealias FILEPointer = OpaquePointer | ||
| #else | ||
| typealias FILEPointer = UnsafeMutablePointer<FILE> |
There was a problem hiding this comment.
Should this be named FilePointer to look a little more swift? It’s bad enough to have FILE scream at you in all caps.
| @@ -198,17 +198,22 @@ class PrintDiags: ParsableCommand { | |||
| @Flag(name: .long, help: "Perform sequence folding with the standard operators") | |||
| var foldSequences: Bool = false | |||
|
|
|||
| @Flag(name: .long, help: "Colorize output with ANSI color codes") | |||
| @Flag(name: .long, help: "Force output coloring with ANSI color codes") | |||
There was a problem hiding this comment.
You could make this a flag with inversion: .prefixedNo like in https://github.com/apple/swift-format/blob/072768f1af50989996e137919f9f6be71e8294e6/Sources/swift-format/Subcommands/LintFormatOptions.swift#L62-L69 so you can also force coloring off if you want to.
And now that I think about it, we should name the flag color-diagnostics (just rename the variable to colorDiagnostics) to match the name of the flag in the Swift Compiler.
| @Flag(name: .long, help: "Force output coloring with ANSI color codes") | |
| @Flag(name: .long, inversion: .prefixedLo, help: "Enable or disable coloring with ANSI color codes") | |
| let colorDiagnostics: Bool? |
|
@ahoppen are you okay with those suggestions being addressed in a follow-up PR? |
|
I'm going to merge because this is progress, and we can address feedback in a follow-up. |
This builds on the work in #1256 to add support for automatically enabling output coloring as suggested by @ahoppen. As of this change when invoking
$ swift-parser-cli print-diagsin a terminal it will enable output coloring by default. The--colorizeflag introduced in #1256 is still supported to force output colors, if desired.API-wise this should be a non-breaking change since it only adds overloads to instantiate a
DiagnosticsFormatterwith a newColorModeparameter (shamelessly stolen from swift-format).Some notes:
stderrwith a few wrapper types to simplify reading/writing to it. This doesn't seem to be used for detecting a TTY environment which is why I left it out for now.TSCLibcmodule that re-exports symbols from C standard libraries on the various platforms (Glibc for Linux, CRT on Windows and Darwin.C on Apple platforms) but opted to "inline" the use ofstderrfor now. Don't have the resources at the moment to test this cross platform and so would more than appreciate any pointers here.ProcessEnvtype to read properties from the environment (relevant in this case is theProcessEnv.vars["TERM"]call to check whether it's set to"dumb") on non-Windows platforms. I've opted to remove support for detecting this case for now since copyingProcessEnvinto swift-syntax seems out of scope but please do let me know if there's anything I'm missing here.Additionally, I've taken the liberty of adding "error:" and "warning:" prefixes to formatted output when coloring is enabled to help with accessibility, as suggested by @DougGregor in #1256. I realize it's slightly out of scope but it's a nice addition in my opinion, let me know if this is better suited for a separate PR.