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

Colorize output from cargo on non-Windows OSes #529

Closed
wants to merge 1 commit into from

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Jan 27, 2019

related: #298

Make sure these boxes are checked! 📦✅

  • You have the latest version of rustfmt installed
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran cargo fmt on the code base before submitting
  • You reference which issue is being closed in the PR text

I found previous attempts at #293 #423. I understood that enabling colors causes a problem on Windows, but I still want to colorize output from cargo because it's much easier to read compilation errors from cargo build.

By this patch, I want to suggest to enable colors on non-Windows OSes. I read #298 and it seems to solve the Windows issue, but there is no update on the issue from Nov. If it would still take time to complete, I want to enable colors at least on non-Windows OSes.

@rhysd rhysd changed the title colorize output from cargo on non-Windows OSes Colorize output from cargo on non-Windows OSes Jan 27, 2019
@ashleygwilliams ashleygwilliams added needs review question Further information is requested labels Jan 28, 2019
@ashleygwilliams
Copy link
Member

@alexcrichton does this have any negative implications that you are aware of? i certainly agree that we need to address #298 but would this be an acceptable mitigation for non windows users in the meantime?

Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

conditionally approving this, but waiting to hear from @alexcrichton on any negative implications this may have that i'm not aware of

@alexcrichton
Copy link
Contributor

I feel the same as before, I wouldn't want to accept a fix which doesn't actually fix one of the major platforms.

@rhysd
Copy link
Contributor Author

rhysd commented Jan 29, 2019

@alexcrichton I understood your concern. I feel you're right. This change may give some impression that Windows support is worse than other platforms.

Since I agree with @alexcrichton 's thought, I'm closing this. I'll apply this patch personally.

@ashleygwilliams @alexcrichton Thank you for your quick reviews.

@rhysd rhysd closed this Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - feature needs review question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants