-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add clippy::manual_let_else
at warn level to lints
#10684
Conversation
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.
Having this lint is good, and it did simplify a lot of code. In particular, let Ok(...)
are all nice changes.
I'm concerned about few cases where variable being assigned is hidden deep inside the destructuring, so it is not obvious from the first glance what it is. uuid_str
being a very egregious example of this. Maybe keep old code in there, disable lint for the statement, and (if you want) open clippy bugreport for emitting bad advice.
@@ -17,9 +17,12 @@ pub(crate) fn type_uuid_derive(input: DeriveInput) -> syn::Result<TokenStream> { | |||
continue; | |||
}; | |||
|
|||
let uuid_str = match &name_value.value { |
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.
This expression became much less readable imho.
uuid_str
is hidden inside a lot of syntax, and it is not immediately clear that it is the value being assigned.
Maybe disable lint just for this one line, or rewrite it to let uuid_str = if (...) {} else {}
statement.
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.
Reverted this one, but for some reason the allow
attribute only works when applied on the upper block level, not on the expression itself 😕
VertexFormat::Float32x3, | ||
)) | ||
} | ||
let VertexAttributeValues::Float32x3(positions) = |
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.
same here, kind of
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.
While I agree for the other since matched value was quite deep in the pattern, here I think that while it introduce a bit of indentation to read the binded variable, it is still clearer than the match
version ?
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.
not sure about this one, do as you like
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.
I guess my problem is with one overly complex statement:
let VertexAttributeValues::Float32x3(positions) =
mesh.attribute(Mesh::ATTRIBUTE_POSITION).ok_or(
GenerateTangentsError::MissingVertexAttribute(Mesh::ATTRIBUTE_POSITION.name),
)?
else {
return Err(GenerateTangentsError::InvalidVertexAttributeFormat(
Mesh::ATTRIBUTE_POSITION.name,
VertexFormat::Float32x3,
));
};
Breaking it into multiple simpler statements might be more readable (here, I'm creating attribute
variable for this purpose):
let attribute = mesh.attribute(Mesh::ATTRIBUTE_POSITION).ok_or(
GenerateTangentsError::MissingVertexAttribute(Mesh::ATTRIBUTE_POSITION.name),
)?;
let VertexAttributeValues::Float32x3(positions) = attribute else {
return Err(GenerateTangentsError::InvalidVertexAttributeFormat(
Mesh::ATTRIBUTE_POSITION.name,
VertexFormat::Float32x3,
));
};
edit: that's called "expression complexity", I've just found a nice article about it:
https://grugbrain.dev/#grug-on-expression-complexity
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 suggestion, done 🙂
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.
I think you should follow rlidwka's suggestion for the vertex attributes, but otherwise LGTM.
Objective
Related to #10612.
Enable the
clippy::manual_let_else
lint as a warning. Thelet else
form seems more idiomatic to me than amatch
/if else
that either match a pattern or diverge, and from the clippy doc, the lint doesn't seem to have any possible false positive.Solution
Add the lint as warning in
Cargo.toml
, refactor places where the lint triggers.