bnjbvr / cargo-machete

Remove unused Rust dependencies with this one weird trick!
MIT License
784 stars 28 forks source link

Replace walkdir with ignore for better path walking defaults #48

Closed mickvangelderen closed 1 year ago

mickvangelderen commented 2 years ago

The ignore crate (also by Burntsushi) has good defaults for walking the file system, like for example not recursing into ignored directories.

On the implementation side, instead of building a parallel iterator through rayon, we could also use the parallel walker exposed by ignore. We will have to implement a visitor type to collect the analysis results though. I opted not to do this in the initial PR to limit the amount of changes.

mickvangelderen commented 2 years ago

How do we guarantee the code works? I have not manually tested this change.

bnjbvr commented 2 years ago

If it passes CI it's a good sign! The last run failed because the code isn't formatted anymore; running cargo fmt should fix it.

bnjbvr commented 2 years ago

Oh and I forgot to say, you can run tests locally with cargo test as well.

bnjbvr commented 2 years ago

uh it's not passing Clippy checks now, sorry :sweat_smile:

clippy --all --all-features -- -D warnings

mickvangelderen commented 2 years ago

@bnjbvr the tests should pass now but there's something to consider in the implementation.

To make the target_dir_is_not_skipped_when_skip_target_dir_is_false test pass we need to both set skip_target_dir to false and disable the usage of git ignore files in the WalkBuilder. That defeats the purpose to introduce the ignore crate.

IgnisDa commented 1 year ago

Hi @mickvangelderen @bnjbvr! Any updates on this PR?

bnjbvr commented 1 year ago

No updates on my side, I was waiting for the PR's author. If we don't hear from them, say, in the next two weeks, I'll close this, unless someone wants to take the PR over (I'm still open to the idea of having that feature,though).

mickvangelderen commented 1 year ago

The proposed change is pretty fun to try and implement so I might give it a try just because it seems there is interest in it now. My own motivation waned because my team did not end up using cargo-machete and I simply prioritized other work.

mickvangelderen commented 1 year ago

I reimplemented this change in #95 .