Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement try_filter_entry #134

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ThinkChaos
Copy link

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

That would basically remove the WalkIterator trait.

@BurntSushi
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants