-
Notifications
You must be signed in to change notification settings - Fork 128
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
Upgrade yansi: 0.5.1 -> 1.0.1 #389
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -249,20 +249,16 @@ fn main() { | |
let args = Cli::parse(); | ||
|
||
// Determine the usage of styles | ||
#[cfg(target_os = "windows")] | ||
let ansi_support = yansi::Paint::enable_windows_ascii(); | ||
#[cfg(not(target_os = "windows"))] | ||
let ansi_support = true; | ||
let enable_styles = match args.color.unwrap_or_default() { | ||
// Attempt to use styling if instructed | ||
ColorOptions::Always => true, | ||
ColorOptions::Always => { | ||
yansi::enable(); // disable yansi's automatic detection for ANSI support on Windows | ||
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. @nc7s Sorry to bother you again, but how do you feel about this change I added? It should now be exactly the previous behavior right? 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. I don't really know enough to judge this, from a glance LGTM? |
||
true | ||
} | ||
// Enable styling if: | ||
// * There is `ansi_support` | ||
// * NO_COLOR env var isn't set: https://no-color.org/ | ||
// * The output stream is stdout (not being piped) | ||
ColorOptions::Auto => { | ||
ansi_support && env::var_os("NO_COLOR").is_none() && io::stdout().is_terminal() | ||
} | ||
ColorOptions::Auto => env::var_os("NO_COLOR").is_none() && io::stdout().is_terminal(), | ||
// Disable styling | ||
ColorOptions::Never => false, | ||
}; | ||
|
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 suppose by https://docs.rs/yansi/1.0.1/yansi/index.html#windows we can omit this. Fair enough
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.
Yeah the commit message explained ;)
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.
Ah, I didn't see that, I'll make sure to include it in the merged commit