BurntSushi / walkdir

Rust library for walking directories recursively.
The Unlicense
1.21k stars 106 forks source link

Implement `try_filter_entry` #134

Open ThinkChaos opened 4 years ago

ThinkChaos commented 4 years ago

I had to add a couple of traits to keep the crate usable, but made sure the user doesn't need to know about them.

It took me a couple of tries to land on the TryFilterPredicate trait, here's a breakdown of my thought process, maybe you'll have a better idea:

  1. The signature fn filter_entry<P>(self, predicate: P) -> FilterEntry<Self, P> has to use P in the return type, so the predicate can't just be wrapped like |res| res.is_err() || predicate(res.unwrap()) otherwise we get an unnameable return type, having to use -> impl Trait syntax which would break method chaining.

  2. I added the TryFilterPredicate trait and tried to implement it for both FnMut(&DirEntry) -> bool and FnMut(&Result<DirEntry>) -> bool, but the compiler says they're conflicting implementations, I don't understand why and didn't find much on this.

  3. So next I tried to add FilterPredicateAdapter<P: FnMut(&DirEntry) -> bool> and implement FnMut(&Result<DirEntry>) for it but that doesn't seem possible on stable yet.

  4. And finally I got it working by implementing TryFilterPredicate for FnMut(&Result<DirEntry>) -> bool, and for FilterPredicateAdapter. So now FilterEntry is defined as struct FilterEntry<I, P>(TryFilterEntry<I, FilterPredicateAdapter<P>>).

I'm still relatively new to writing rust, so please point out any changes I can make to improve the code, naming, docs, or anything else!

ThinkChaos commented 4 years ago

I could add a rust-toolchain file to the root to force cargo to use Rust 1.34.0 if you want, but somehow cargo fails to install it on my machine so...

BurntSushi commented 4 years ago

Wow, okay, it looks like you put a lot of work into this. Unfortunately, the public API changes appear quite voluminous and it's not actually easy for me to tell at a glance whether there are any breaking changes. If this is really the only way to add try_filter_entry, then I'd probably have to decline on the feature altogether because I don't think it's worth this complexity.

Can you say why you diverged from my suggestion in #131? That is, can you just add a new try_filter_entry method to IntoIter and FilterEntry?

ThinkChaos commented 4 years ago

I understand. I don't think I introduced any breaking changes, and the old tests still compile without any changes.

That's what I tried to do, maybe I misunderstood: I duplicated FilterEntry as TryFilterEntry, got it working and then changed FilterEntry to wrap that. Maybe removing the ability to compose the iterators would be enough simplification. It is now possible to do:

WalkDir::new(dir.path())
    .into_iter()
    .filter_entry(|_| todo!())
    .try_filter_entry(|_| todo!());
ThinkChaos commented 4 years ago

That would basically remove the WalkIterator trait.

BurntSushi commented 4 years ago

Yeah I think the introduction of a new trait is what is concerning to me. But yeah, I agree, the lack of a trait makes composition impossible, which isn't great either. Not sure what to do. I'll have to noodle on it.