Skip to content

Don't assume color=always when explicitally specified #31550

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Feb 12, 2016

Conversation

Stebalien
Copy link
Contributor

Fixes #31546

@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@Stebalien
Copy link
Contributor Author

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned alexcrichton Feb 11, 2016
@Stebalien
Copy link
Contributor Author

Note: I haven't actually tested this change because rust takes an eternity to build on my poor old laptop. However, I don't expect it to fail (famous last words...).


Some(arg) => {
early_error(ErrorOutputType::default(), &format!("argument for --error-format must \
early_error(ErrorOutputType::HumanReadable(color), &format!("argument for --error-format must \
Copy link
Member

Choose a reason for hiding this comment

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

Overlong line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

early_error(ErrorOutputType::HumanReadable(color),
&format!("argument for --error-format must \
be human or json (instead was \
`{}`)",
Copy link
Member

Choose a reason for hiding this comment

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

nit: this can squeeze on the previous line.

@nrc
Copy link
Member

nrc commented Feb 11, 2016

r+ with the style nit addressed

@nrc
Copy link
Member

nrc commented Feb 11, 2016

@bors: r+

@bors
Copy link
Collaborator

bors commented Feb 11, 2016

📌 Commit 03ef55b has been approved by nrc

@alexcrichton
Copy link
Member

Thanks for the quick fix @Stebalien!

Nominating for backport as well

@alexcrichton alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 11, 2016
@durka
Copy link
Contributor

durka commented Feb 11, 2016

Can we test this somehow to prevent future regressions? Maybe with a run-make test that greps for \x1b or something?

@bors
Copy link
Collaborator

bors commented Feb 12, 2016

⌛ Testing commit 03ef55b with merge 0c4d81f...

bors added a commit that referenced this pull request Feb 12, 2016
@pnkfelix
Copy link
Member

@durka I agree, it seems like testing this via run-make would be good. Adding E-needstest flag...

Update: (or maybe I should have put the flag on #31546 ... either way, I suspect bors is going to close the PR and issue regardless of what labels I add at this point...)

@pnkfelix pnkfelix added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Feb 12, 2016
@bors bors merged commit 03ef55b into rust-lang:master Feb 12, 2016
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. beta-accepted Accepted for backporting to the compiler in the beta channel. labels Feb 24, 2016
@nikomatsakis
Copy link
Contributor

Accepting for beta since seems awfully minor.

@nikomatsakis
Copy link
Contributor

cc @rust-lang/compiler

@arielb1
Copy link
Contributor

arielb1 commented Feb 25, 2016

+1

@brson brson mentioned this pull request Feb 26, 2016
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 26, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants