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

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

Merged
merged 1 commit into from
Feb 12, 2016

Conversation

Stebalien
Copy link
Contributor

Fixes #31546

@rust-highfive
Copy link
Collaborator

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
Contributor

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
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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