SuperCuber / dotter

A dotfile manager and templater written in rust 🦀
The Unlicense
901 stars 48 forks source link

Minor cleanup, resolve some Clippy lints #155

Closed Diomendius closed 7 months ago

Diomendius commented 11 months ago

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.

SuperCuber commented 11 months ago

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

Diomendius commented 7 months ago

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.

SuperCuber commented 7 months ago

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

Diomendius commented 7 months ago

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. 😅

SuperCuber commented 7 months ago

THanks for the contribution :D