minor: fix some clippy lints #10135
minor: fix some clippy lints #10135
Conversation
| | cargo_metadata::Message::BuildFinished(_) | ||
| | cargo_metadata::Message::TextLine(_) | ||
| | _ => (), | ||
| _ => (), |
There was a problem hiding this comment.
It's fine, it makes sense, but the default branch was a bit weird (we might want to know if new message types are added).
There was a problem hiding this comment.
Well we won't know that though because due to non exhaustiveness we have to use a wildcard no matter what
There was a problem hiding this comment.
Yeah, it's was an "ugh" for the old code, not for your change.
There was a problem hiding this comment.
Upstream added non-exhaustive, that's why we need this here. Last weekend I almost got to prototyping miniserde-based cargo data to have more control over this :-)
| if diagnosed_extern_crates.contains(krate) { | ||
| continue; | ||
| } | ||
| if let (Some(krate), PathKind::Plain | PathKind::Abs) = |
There was a problem hiding this comment.
how so? It doesn't really change the nesting level, though on another note match + if guard seems like the proper way here
There was a problem hiding this comment.
I think matklad generally prefers match vs. if let: https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/style.md#if-let
There was a problem hiding this comment.
But this isn't an if let ... {} else {}, just an if let
There was a problem hiding this comment.
that was indeed intentional -- multiline conditions are not readable. In
if let (Some(krate), PathKind::Plain | PathKind::Abs) =
(directive.import.path.segments().first(), &directive.import.path.kind)you need to read a lot to understand what the heck are you even matching. With match, scrutinee comes first, which I personally find easier on the eyes: pattern in isolation doesn't make much sense, it only is meaningful with respect to a specific expression you pattern match.
But no strong opinion here!
| if !fn_traits.contains(&assoc_data.trait_id) { | ||
| return None; | ||
| } | ||
| if let WhereClause::AliasEq(AliasEq { alias: AliasTy::Projection(projection), ty }) = |
There was a problem hiding this comment.
For what reason? To me collapsing the matches seems better personally
There was a problem hiding this comment.
ditto: here, a complicated pattern comes before the expression to match.
|
bors r- |
|
Canceled. |
|
bors r+ |
| | cargo_metadata::Message::BuildFinished(_) | ||
| | cargo_metadata::Message::TextLine(_) | ||
| | _ => (), | ||
| _ => (), |
There was a problem hiding this comment.
Upstream added non-exhaustive, that's why we need this here. Last weekend I almost got to prototyping miniserde-based cargo data to have more control over this :-)
| if diagnosed_extern_crates.contains(krate) { | ||
| continue; | ||
| } | ||
| if let (Some(krate), PathKind::Plain | PathKind::Abs) = |
There was a problem hiding this comment.
that was indeed intentional -- multiline conditions are not readable. In
if let (Some(krate), PathKind::Plain | PathKind::Abs) =
(directive.import.path.segments().first(), &directive.import.path.kind)you need to read a lot to understand what the heck are you even matching. With match, scrutinee comes first, which I personally find easier on the eyes: pattern in isolation doesn't make much sense, it only is meaningful with respect to a specific expression you pattern match.
But no strong opinion here!
| if !fn_traits.contains(&assoc_data.trait_id) { | ||
| return None; | ||
| } | ||
| if let WhereClause::AliasEq(AliasEq { alias: AliasTy::Projection(projection), ty }) = |
There was a problem hiding this comment.
ditto: here, a complicated pattern comes before the expression to match.
| _ => { | ||
| // FIXME report error (ambiguous associated type) | ||
| (TyKind::Error.intern(&Interner), None) | ||
| } |
3 participants
bors r+