Skip to content
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

Autodetect TTY and add colors #394

Merged
merged 2 commits into from
May 6, 2016
Merged

Conversation

peschkaj
Copy link
Contributor

@peschkaj peschkaj commented May 2, 2016

If *nix users are running on a tty (using the same checks as rustc & cargo), then colors will be displayed.

Windows users are still out in the cold. This could potentially be fixed by changing rustc to emit ANSI color codes on Windows 10 and newer - but that's a rustc thing, not a rustup thing.

let e = e.as_ref().to_str().unwrap_or("");
e == "--color"
})
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please pull out this expression from (&args).iter() onward into a local variable with a descriptive name like, has_color_arg.

I believe this check should be e.starts_with("--color") since it could see e.g. --color=always.

@brson
Copy link
Contributor

brson commented May 4, 2016

Looks good. Thank you.

@peschkaj
Copy link
Contributor Author

peschkaj commented May 5, 2016

Thanks for the eyeballs! Fixed those issues, test pass locally, now to wait for appveyor and travis.

@brson
Copy link
Contributor

brson commented May 5, 2016

lgtm but the test failure is legit

@peschkaj peschkaj force-pushed the 378-rustc-colors branch from c2663a0 to 4e616ef Compare May 5, 2016 18:43
@peschkaj
Copy link
Contributor Author

peschkaj commented May 5, 2016

Should be fixed in the current commit. This is the reverse of the usual pain - I frequently fail to test on Linux.

@brson brson merged commit 2da4bef into rust-lang:master May 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants