dotter icon indicating copy to clipboard operation
dotter copied to clipboard

Minor cleanup, resolve some Clippy lints

Open Diomendius opened this issue 1 year ago • 1 comments

I ran cargo clippy -- -W clippy::pedantic and fixed the issues it complained about that I thought were most reasonable. I also removed a redundant clone from watch.rs which Clippy surprisingly didn't complain about.

There were a few lints I decided against fixing, such as a |p| p.render() closure in handlebars_helpers.rs which would need to be expanded to the fully-qualified path handlebars::PathAndJson::render. There were a lot of instances of clippy::uninlined-format-args (fmt!("{} {}", x, y) → fmt!("{x} {y}")) which I didn't bother with, though if you prefer the inline style, you could run cargo clippy --fix -- -A clippy::all -W clippy::uninlined-format-args to auto-fix them.

I also noticed the crate is on edition = "2018". Changing this to edition = "2021" doesn't seem to require any other changes to the code, so it should be an easy change if you want to do that at some point.

Diomendius avatar Dec 16 '23 08:12 Diomendius

Looks good. I like the inline format args and the edition suggestions, could you add them to the PR?

SuperCuber avatar Dec 21 '23 07:12 SuperCuber

I think I wrongly assumed you would be notified when I pushed commits, so this has sat open for longer than I expected. Sorry about that.

Anyway, I've revisited this, rebased to resolve the minor conflict in handlebars_helpers.rs, fixed some more lints and fixed a compile error I found along the way which would occur when running cargo test --no-default-features. I suppose the compile error is slightly out of scope for this PR, but the fix only adds two #[cfg] annotations, so I've included it with everything else.

This doesn't quite resolve everything that Clippy complains about with -W clippy::pedantic, but what remains is dubiously helpful. You can have a look yourself with cargo clippy --tests -- -W clippy::pedantic if you like.

Diomendius avatar Apr 12 '24 05:04 Diomendius

Looks great :D I commented on the .is_err() assert but I assume there might be more asserts like it, or .is_ok, .is_some etc. Please replace them with .unwrap and .unwrap_err :)

Then it's ready for merge

SuperCuber avatar Apr 13 '24 09:04 SuperCuber

I actually couldn't find any other uses of .is_err(), .is_ok(), .is_some() or .is_none() in an assert, but I pushed a commit to fix the one you noticed.

And, since I happened to notice, for good measure I also renamed the config::tests module to test, since it was the only one not following that naming convention. I swear I didn't set out to make a bunch of single-character commits on purpose. 😅

Diomendius avatar Apr 13 '24 13:04 Diomendius

THanks for the contribution :D

SuperCuber avatar Apr 13 '24 13:04 SuperCuber