-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Split ColorConfig
off of HumanReadableErrorType
#128806
Conversation
This comment has been minimized.
This comment has been minimized.
3966c54
to
9ee3a14
Compare
This comment has been minimized.
This comment has been minimized.
r? jieyouxu |
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.
Good cleanup! Just a typo and a tiny nit, feel free to r=me afterwards.
config::ErrorOutputType::HumanReadable(kind) => { | ||
let (short, color_config) = kind.unzip(); | ||
config::ErrorOutputType::HumanReadable(kind, color_config) => { | ||
let short = matches!(kind, HumanReadableErrorType::Short); |
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.
Nit: very tiny nit: this matches!(kind, HumanReadableErrorType::Short)
is repeated in quite a few places. Since HumanReadableErrorType
derives PartialEq
, this could just be
kind == HumanReadableErrorType::Short
or maybe if we want the usage site to look like kind.short()
impl HumanReadableErrorType {
fn short(self) -> bool { self == HumanReadableErrorType::Short }
}
The previous setup tied two unrelated things together. Splitting these two is a better model.
1a0c155
to
4447880
Compare
This comment has been minimized.
This comment has been minimized.
@bors r=jieyouxu |
Split `ColorConfig` off of `HumanReadableErrorType` The previous setup tied two unrelated things together. Splitting these two is a better model. Identified by https://github.com/rust-lang/rust/pull/126597/files#r1667800754
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#128640 (rwlock: disable 'frob' test in Miri on macOS) - rust-lang#128791 (Don't implement `AsyncFn` for `FnDef`/`FnPtr` that wouldnt implement `Fn`) - rust-lang#128806 (Split `ColorConfig` off of `HumanReadableErrorType`) - rust-lang#128818 (std float tests: special-case Miri in feature detection) - rust-lang#128834 (rustdoc: strip unreachable modules) - rust-lang#128836 (rustdoc-json: add a test for impls on private & hidden types) - rust-lang#128837 (Clippy subtree update) - rust-lang#128851 (Add comment that bors did not see pushed before it merged) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#128806 - estebank:color-config, r=jieyouxu Split `ColorConfig` off of `HumanReadableErrorType` The previous setup tied two unrelated things together. Splitting these two is a better model. Identified by https://github.com/rust-lang/rust/pull/126597/files#r1667800754
Upgrades toolchain to 08/28 Culprit upstream changes: 1. rust-lang/rust#128812 2. rust-lang/rust#128703 3. rust-lang/rust#127679 4. rust-lang/rust-clippy#12993 5. rust-lang/cargo#14370 6. rust-lang/rust#128806 Resolves #3429 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
The previous setup tied two unrelated things together. Splitting these two is a better model.
Identified by https://github.com/rust-lang/rust/pull/126597/files#r1667800754