-
Notifications
You must be signed in to change notification settings - Fork 440
Implement support for automatically coloring output in DiagnosticsFormatter #1280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,10 +139,11 @@ public struct DiagnosticsFormatter { | |
switch message.severity { | ||
case .error: | ||
let annotation = ANSIAnnotation(color: .red, trait: .bold) | ||
return annotation.applied(to: message.message) | ||
return annotation.applied(to: "error: \(message.message)") | ||
case .warning: | ||
let annotation = ANSIAnnotation(color: .yellow) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also add a prefix for notes? |
||
} | ||
|
@@ -181,6 +182,10 @@ struct ANSIAnnotation { | |
self.trait = trait | ||
} | ||
|
||
func withTrait(_ trait: Trait) -> Self { | ||
return ANSIAnnotation(color: self.color, trait: trait) | ||
} | ||
|
||
func applied(to message: String) -> String { | ||
// Resetting after the message ensures that we don't color unintended lines in the output | ||
return "\(code)\(message)\(ANSIAnnotation.normal.code)" | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,51 @@ | ||||||||||||||
//===----------------------------------------------------------------------===// | ||||||||||||||
// | ||||||||||||||
// This source file is part of the Swift.org open source project | ||||||||||||||
// | ||||||||||||||
// Copyright (c) 2014 - 2022 Apple Inc. and the Swift project authors | ||||||||||||||
// Licensed under Apache License v2.0 with Runtime Library Exception | ||||||||||||||
// | ||||||||||||||
// See https://swift.org/LICENSE.txt for license information | ||||||||||||||
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||||||||||||||
// | ||||||||||||||
//===----------------------------------------------------------------------===// | ||||||||||||||
|
||||||||||||||
#if canImport(Glibc) | ||||||||||||||
import Glibc | ||||||||||||||
#elseif os(Windows) | ||||||||||||||
import CRT | ||||||||||||||
#else | ||||||||||||||
import Darwin.C | ||||||||||||||
#endif | ||||||||||||||
|
||||||||||||||
#if os(Android) | ||||||||||||||
typealias FILEPointer = OpaquePointer | ||||||||||||||
#else | ||||||||||||||
typealias FILEPointer = UnsafeMutablePointer<FILE> | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be named |
||||||||||||||
#endif | ||||||||||||||
|
||||||||||||||
enum TerminalHelper { | ||||||||||||||
static var isConnectedToTerminal: Bool { | ||||||||||||||
return isTTY(stderr) | ||||||||||||||
} | ||||||||||||||
Comment on lines
+28
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 And, to be clear, we should probably name the method to mention that is checking
Suggested change
|
||||||||||||||
|
||||||||||||||
/// Checks if passed file pointer is a tty. | ||||||||||||||
static func isTTY(_ filePointer: FILEPointer) -> Bool { | ||||||||||||||
return terminalType(filePointer) == .tty | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// The type of terminal. | ||||||||||||||
enum TerminalType { | ||||||||||||||
/// The terminal is a TTY. | ||||||||||||||
case tty | ||||||||||||||
|
||||||||||||||
/// The terminal is a file stream. | ||||||||||||||
case file | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// Computes the terminal type of the stream. | ||||||||||||||
static func terminalType(_ filePointer: FILEPointer) -> TerminalType { | ||||||||||||||
let isTTY = isatty(fileno(filePointer)) != 0 | ||||||||||||||
return isTTY ? .tty : .file | ||||||||||||||
} | ||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could make this a flag with And now that I think about it, we should name the flag
Suggested change
|
||||||||
var colorize: Bool = false | ||||||||
|
||||||||
func run() throws { | ||||||||
let source = try getContentsOfSourceFile(at: sourceFile) | ||||||||
|
||||||||
source.withUnsafeBufferPointer { sourceBuffer in | ||||||||
let tree = Parser.parse(source: sourceBuffer) | ||||||||
|
||||||||
var diags = ParseDiagnosticsGenerator.diagnostics(for: tree) | ||||||||
print(DiagnosticsFormatter.annotatedSource(tree: tree, diags: diags, colorize: colorize)) | ||||||||
let annotatedSource = DiagnosticsFormatter.annotatedSource( | ||||||||
tree: tree, | ||||||||
diags: diags, | ||||||||
colorize: colorize || TerminalHelper.isConnectedToTerminal | ||||||||
) | ||||||||
|
||||||||
print(annotatedSource) | ||||||||
|
||||||||
if foldSequences { | ||||||||
diags += foldAllSequences(tree).1 | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.