Clippy lints#14
Conversation
chrysn
left a comment
chrysn
left a comment
There was a problem hiding this comment.
I'm not a big fan of outright forbidding min_ident_chars, see comment.
The others are good: Making cast_possible_truncation is done well narrowly, and using expect over allow is practical for ensuring that they don't linger beyond their usefulness.
| allow_attributes = "warn" | ||
| cargo_common_metadata = "warn" | ||
| let_underscore_untyped = "warn" | ||
| min_ident_chars = "warn" |
There was a problem hiding this comment.
I think linting on this is excessive. If the lint had a threshold of lines around the definition where a short name is used (eg. definition +2 lines, and all cases in here ), maybe revisit, but things like match { …, Err(e) => do_something_with(e) } IMO don't need the verbosity of a longer name.
There was a problem hiding this comment.
The lint ignores some common variable names like x and y.
I think its bad practice to use short variable names at all. It does not end up in the compiled target, so there is no point in using short variable names. Editors with auto-completion exist and here in this repo it's already starting with some weird one letter things. Not sure what kind of content exists in Slipmux::Diagnostic but s definitely does not help me to understand that. Preventing one letter variable names would have prevented that unreadable code earlier. It's just more readable.
I can remove the commit from this PR, but I still think it's a good addition.
There was a problem hiding this comment.
I don't quite see why this is better for x and y -- the mental pattern is the same: If I read .any(|s| s.is_upper()), it has the ring of a mathematical "for-all" statement where just as well, there is no strong incentive to give a full name to s.
If it'd convey information to the reader they don't have from the context, that'd be fine, but that's not the case in many of the changes here:
Err(e)is a relatively common idiom; granted, that's only the pattern being matched in 1 of 2 occurrences ofe, but that is used inErr(e)just in the one place it is used.scan be an indicator of stringishness in short variables (not a strong one, just as not everyxis real and not everyiis integer), without being specific about whether it's &str, String or something different. Actually the place where the patch putsstrit'd be aString(and/or in another place a&String, but that's not a useful property to a reader, who for when it does matter can ask their editor).
I can remove the commit from this PR, but I still think it's a good addition.
To a large extent I think this is a matter of subjective style; @Teufelchen1 will need to be the judge of it, and I half expect him to side with you here. I merely came by here under my "don't nag people about reviewing my PRs unless I've helped them review others' PRs" guideline ;-)
There was a problem hiding this comment.
This is a difficult decision.
I am very confused that the default allowed variables do no contain e. The Err(e) pattern is so common and hard to confuse what e refers to.
Having this lint always on & enforced is probably annoying with too little benefit. It could provide more value as a static test for reviews. Having ignore-able warnings about too short variables names during a review might help the reviewer to not forget about this aspect?
There was a problem hiding this comment.
Personally I don't think the set of default allowed variables is a good idea in the first place as stuff like i can also be seen as annoying. Attempting such a set will always be wrong…
Personally I would also like i to be warned about, but I seem to be more against single letter variables than others here 🙃
the mental pattern is the same: If I read
.any(|s| s.is_upper()), it has the ring of a mathematical "for-all" statement where just as well, there is no strong incentive to give a full name tos.
I prefer using the methods without the closure: .any(char::is_uppercase) (also see https://rust-lang.github.io/rust-clippy/stable/index.html#redundant_closure_for_method_calls)
There was a problem hiding this comment.
I agree with Clippy's statement on the level here: That closure lint is pedantic. The compiler is smart enough to process it either way, and while it's still reasonably readable for char, I may not know the type name off head (something something stringish), and other types have way more complex names which using a closure avoids.
I also agree with you on the set: That'll have cases where i is bad as well. Personally, I'd rather have a check about whether it's used across more than 2 lines after definition.
Anyway: Either style works for me, and I like the fixup updates.
There was a problem hiding this comment.
I agree with Clippy's statement on the level here: That closure lint is pedantic. The compiler is smart enough to process it either way, and while it's still reasonably readable for char, I may not know the type name off head (something something stringish), and other types have way more complex names which using a closure avoids.
Personally I often just write the closure and accept the lint suggestion replacing it by the function itself. Then it's easier to read as its then clear what exactly happens there: any character which is uppercase. And there is no need for a variable then at all as everything is clear from the method name already.
| allow_attributes = "warn" | ||
| cargo_common_metadata = "warn" | ||
| let_underscore_untyped = "warn" | ||
| min_ident_chars = "warn" |
There was a problem hiding this comment.
This is a difficult decision.
I am very confused that the default allowed variables do no contain e. The Err(e) pattern is so common and hard to confuse what e refers to.
Having this lint always on & enforced is probably annoying with too little benefit. It could provide more value as a static test for reviews. Having ignore-able warnings about too short variables names during a review might help the reviewer to not forget about this aspect?
3 participants
No description provided.