Enforce selected clippy lints, and small style fixes #4570
Conversation
samueltardieu
force-pushed
the
cleanups
branch
2 times, most recently
from
b79ed65
to
f0c8f9f
Compare
Most collection references implement `.into_iter()` or its mutable version, so it is possible to iterate over the elements without using an explicit method to do so.
`.clone()` is more explicit when we already have an object of the right type.
samueltardieu
force-pushed
the
cleanups
branch
from
f0c8f9f
to
bbbbc4a
Compare
|
This is not necessarily related to this PR, but |
There was a problem hiding this comment.
I like most of these, except for the redundant_else commit (as you foresaw). I highlighted the two that seemed to get a lot worse. The others in that commit seem OKish to change, though I often am not sure about them and I'm not sure it's good to force those changes.
So, I'd prefer dropping that entire commit, but I don't insist on it (especially if the two cases I highlighted can be avoided)
I'm happy with the others, though I'd wait for others to take a look.
| @@ -92,13 +92,13 @@ pub fn relative_path(from: &Path, to: &Path) -> PathBuf { | |||
| // Find common prefix. | |||
| for (i, base) in from.ancestors().enumerate() { | |||
| if let Ok(suffix) = to.strip_prefix(base) { | |||
| if i == 0 && suffix.as_os_str().is_empty() { | |||
| return ".".into(); | |||
| return if i == 0 && suffix.as_os_str().is_empty() { | |||
| @@ -212,12 +212,12 @@ impl OpStore for SimpleOpStore { | |||
| let hex_prefix = prefix.hex(); | |||
| if hex_prefix.len() == OPERATION_ID_LENGTH * 2 { | |||
| // Fast path for full-length ID | |||
| if matches_root || op_dir.join(hex_prefix).try_exists()? { | |||
| return Ok(if matches_root || op_dir.join(hex_prefix).try_exists()? { | |||
There was a problem hiding this comment.
I'm ~Ok with this here, but I like the original a bit better, and I don't want this to be enforced by clippy.
|
Another thing to consider is that this will add to headache of us being affected by clippy false-positive bugs. I imagine these checks are less well-tested than the default checks. On the bright side, these used to occur previously, and haven't for the past ~3 months, so perhaps Clippy changed something about their testing. But again, they might be more lax about less frequently used checks. I don't think this is a blocker; if it becomes a problem, we can revert parts of this PR. |
Right now, we only use the basic Clippy lints. While many software opt to use
clippy::pedantic, with sometimes individually disabled restrictions, this would be a bit extreme to do in one step (or to do at all, some lints might be intrusive or opiniated).This PR proposes the activation of selected Clippy lints. Those I have tried so far are:
explicit_iter_loop: prefer iterating over&collectionrather thancollection.iter(), this is more compact and idiomaticflat_map_option: using.flat_map(f)whenfreturns anOptionis equivalent to using.filter_map(f), except that the latter better underlines the filtering aspectimplicit_clone: using.to_vec()on aVec, or.to_owned()on an owned object can be replaced by.clone(), which makes it clearer that we already have something of the right type; at least one.clone()call is even superfluous after doing this transformationneedless_for_each: for simple loops (one occurrence currently in the code), using an explicitforloop is clearer than using.for_each()redundant_else: we learn very early that usingelseafter a block which returns early is useless. This might be the most contentious lint in this list, as it changes the style a bit.semicolon_if_nothing_returned: using;at the end of expressions returning()when we do not explicitly need this unit value better expresses that we have a statement, not just an expression. It increases style consistency.unlined_format_args: inline variable names in{}when interpolating, also for consistency as this is done in 90%+ of the code.Other lints might be of interest of course, but I gave it a first run in order to test the idea, and find some lints that other developers might feel comfortable with.
If I'm not wrong, all those lints are available in the stable compiler used to run Clippy in the CI.
Note: there is also a temporary commit to fix codespell on the CI, it will be merged separately, cf. #4571