-
Notifications
You must be signed in to change notification settings - Fork 86
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
Better ANSI support detection. #43
Conversation
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.
I have one comment above. Do you know if there is a radar for Xcode reporting to be an TTY but not supporting ANSI colour codes?
I'm not entirely sure on the behaviour, I didn't manage to find a formal specification for TTY/ANSI codes.
@@ -12,12 +12,27 @@ protocol ANSIConvertible : Error, CustomStringConvertible { | |||
|
|||
extension ANSIConvertible { | |||
func print() { | |||
if isatty(fileno(stderr)) != 0 { | |||
// Check if Xcode Colors is installed and enabled. |
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.
From what I understand, it's not possible to use this plugin with Xcode 8? Since Commander is Swift 3. It's not possible to use Commander with XcodeColors so I don't think we need this check.
Not that I know of (I did a quick google and openradar search) |
return nil | ||
} | ||
return String(cString: value) | ||
} |
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.
Nitpicking, but not sure it's worth having that private function anymore since XcodeColors
isn't checked anymore and only the TERM
env var has to be read
So using this directly for line 16 would suffice 😛
let termType = getenv("TERM").flatMap({ String(cString: $0) })
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.
This won't work since getenv
returns UnsafeMutablePointer
not an optional.
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.
Right, so add the method back in again? It is clear what it does, and nicely wraps around unsafe pointer stuff.
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.
🤔 not sure to understand how the method we removed worked then… does if let value = getenv(…)
magically do something special with UnsafeMutablePointer
?
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.
Oh, seems UnsafeMutablePointer
is actually force-unwrapped (!
). So there is no optional, but if let would still work.
In SwiftGen we've just hit an issue with
ANSI support
detection in the Xcode 8 console:SwiftGen/SwiftGen#221
While trying to find a solution, I found the Rainbow library, which has a bit more expansive tty detection:
https://github.com/onevcat/Rainbow/blob/master/Sources/OutputTarget.swift
This PR simply incorporates the extra detection into Commander.