Repository navigation
Conversation
| /// | ||
| /// Panics if the directories depth overflows `usize`. | ||
| pub struct WalkDir { | ||
| pub struct WalkDir<State = Unfiltered> { |
There was a problem hiding this comment.
This is a breaking change, contrary to the PR description. Code compiling on 2.1.0 no longer compiles:
fn build(p: &str) -> WalkDir {
WalkDir::new(p).filter(|_| async { Filtering::Continue })
}
// error[E0308]: expected `WalkDir<Unfiltered>`, found `WalkDir<Filtered>`The same applies to struct fields typed WalkDir, wd = wd.filter(..) reassignments, and calling .filter() twice.
The typestate is only needed because the stream is built eagerly in each builder method. Suggestion: store the options and the filter in WalkDir (the filter can be boxed as Box<dyn FnMut(DirEntry) -> BoxedFut<Filtering> + Send>, a type already used in this file), and build the stream lazily on the first poll_next. Then contents_first and filter can be called in any order, and the generic parameter, the Unfiltered / Filtered markers and the State: Unpin bound can all be removed.
There was a problem hiding this comment.
I tried using the file and it resulted in API change for me, but maybe I missed something. Will try that approach.
Regarding the breaking change: cargo-semver check said it is fine, but maybe I used it wrong. The desire to not have breaking changes was the reason for all the typestate shenanigans but oh well
thanks for the feedback
| None => { | ||
| dirs.pop(); | ||
| walk(dirs, filter).await | ||
| let Some(mut popped) = dirs.pop() else { |
There was a problem hiding this comment.
The else branch is unreachable: we are inside if let Some(..) = dirs.last_mut(), so dirs.pop() always returns Some. Please use expect/unreachable! or restructure.
| let Some(mut popped) = dirs.pop() else { | ||
| return walk(dirs, filter, opts).await; | ||
| }; | ||
| if let Some(deferred_entry) = popped.deferred_files.pop() { |
There was a problem hiding this comment.
pop() yields files in reverse read_dir order. For example, with read_dir returning f2, f0, f1, the walk yields f1, f0, f2. The order is unspecified anyway, but the reversal is surprising. It disappears if files are no longer deferred; otherwise use a VecDeque with pop_front.
| return walk(dirs, filter, opts).await; | ||
| }; | ||
| if let Some(deferred_entry) = popped.deferred_files.pop() { | ||
| dirs.push(popped); |
There was a problem hiding this comment.
After read_dir.next() returned None, the item is pushed back and the next poll calls read_dir.next() again on the exhausted ReadDir (line 282). With async-fs, that means another blocking thread-pool round trip for each deferred file. Please drain the deferred items without re-polling (e.g. read_dir: Option<ReadDir> or an exhausted flag), or stop deferring files, which removes the issue.
| }; | ||
|
|
||
| match filtering { | ||
| Filtering::IgnoreDir => walk(dirs, filter, opts).await, |
There was a problem hiding this comment.
Behavior change: previously read_dir was still called on IgnoreDir directories, so an unreadable ignored directory produced an error. It is now skipped entirely. I think the new behavior is better, but please mention it in the PR description / changelog.
| }) | ||
| } | ||
| #[test] | ||
| fn contents_first() -> Result<()> { |
There was a problem hiding this comment.
Nit: a blank line is missing before #[test].
This test only has a single file per directory, so it would not catch the reversed order. Please also add tests for:
contents_first(true)combined withfilter, withIgnoreandIgnoreDiron directories,- multiple files in the same directory (compare sorted or as a set, rather than relying on
read_dirorder), contents_first(false)as a no-op.
|
Thanks for the contribution @BaerLKR. Post-order traversal is a useful thing to have, and the filter semantics look right. I checked out the branch: tests and clippy pass. A local probe confirmed that Ignore , IgnoreDir and Continue all behave correctly together with contents_first(true) . There are a few things to address before merging. |
|
@BaerLKR : Since your pull request I took the opportunity to revisit a long standing bug I had in #13. I did a heavy fix (internal code only, no api change) that changed the recursion algorithm with much improved performances. The fix also handle ignored dir better, as you do in your PR. Please rebase from version 2.2.0. I am sorry for the additional work on your side. |
This currently only works if the filter is applied after enabling post-order traversal
|
Hi @BaerLKR, I am adding more internal changes to the crate to support a more secure version (see my latest commits). Also I would prefer not to go with your implementation that exposes a parametric structure (or template as you like) to the public interface. I would rather follow the same design as the sync WalkDir crate, having an inner structure for configuration parameters, and using the builder pattern for passing configuration options. Give me a bit of time to finish the internal restructuring, and come back to you with another proposal for your request. |
|
Hey, |
2 participants
Hello,
while working on another open source project where we move from blocking directory walk to async-stream based work we needed a contents_first equivalent, like in the sync WalkDir crate. So I decided to implement it upstream.
There shouldn't have been a breaking API change due to this.
Feel free to ask for improvements/qualifications as you see fit.
Thanks :)