-
Notifications
You must be signed in to change notification settings - Fork 106
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
explore traversal that yields directory entry twice #22
Comments
I should be able to take a look over the next few days, and see if I can
fix things. Can you give an exact example of the behaviour you expect so I
can make sure I fix the right thing :-)
Mark
…On Tue, May 30, 2017 at 7:49 PM, windwardly ***@***.***> wrote:
This is about issue #18 <#18>
and PR #19 <#19> . Aside: the
more I dug in, the more I learned, and I'm grateful to all involved for the
work on walkdir.
@mcharsley <https://github.com/mcharsley>, I played around with
contents_first, and for the most part, liked it.
However, it doesn't seem to work with filter_entry() or
skip_current_directory() -- the yielded entries look to be skipping the
remainder of the directory the skipped directory is in, as opposed to the
skipped directory.
Additionally, I was looking for an option which also included the initial
directory.
I see @michaelsproul <https://github.com/michaelsproul> proposed
something which yields both edges, and I played around with a modified
version
<windwardly/walkdir@dbd0266>
of that and seemed to be able to do what I was looking for.
I also played around with writing an iterator adapter
<windwardly/walkdir@d849799>
which also works okay, but didn't feel quite right, as it didn't work to
implement WalkDir, so skip_current_directory was outside of that.
I'm torn between a simpler walkdir plus another crate for an adapter
controlling when and how often a directory is listed, and an integrated
version, making use of the stacks that are already there. I could see
either way, either rolling back the contents_first commit or adding to it
to list the directory twice and have it working with filtering.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALbIF9OtOVDlV-fdLKbpjb5g6l6qx1vqks5r_GTEgaJpZM4NqsoN>
.
|
How's this line of thinking? Since contents_first already yielded the contents of the directory, I would expect the handling of the directory to be the same as how files are handled: filter_entry would just ignore the directory entry, and skip_current_directory would skip the remainder of the directory. With this view, For the I made a contents_first_filter branch with 1 failing and 1 passing test. |
Your test looks reasonable. Not sure why you'd want to call
skip_current_dir in a contents-first mode, mind - but your test describes
what I'd expect if you did. :-)
I'll try and take a look at filter_entry some time this week.
Mark
…On Wed, May 31, 2017 at 11:42 PM, windwardly ***@***.***> wrote:
... an exact example of the behaviour you expect... ?
How's this line of thinking?
For files, filter_entry filters only specific files, and for
skip_current_dir filters the rest of the directory.
Since contents_first already yielded the contents of the directory, I
would expect the handling of the directory to be the same as how files are
handled: filter_entry would just ignore the directory entry, and
skip_current_directory would skip the remainder of the directory.
With this view, skip_current_dir works and filter_entry currently doesn't.
For the filter_entry case, if filtering all directories, then all files
would be listed.
I made a contents_first_filter
<https://github.com/windwardly/walkdir/tree/contents_first_filter> branch
with 1 failing and 1 passing test
<windwardly/walkdir@e85266a>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALbIF5aXnOCL3IfS0O2Jp27N5CzNTRmuks5r_ezOgaJpZM4NqsoN>
.
|
Thanks. I wasn't looking to call skip_current_dir as much as I was looking for a model which I could predict how things work, that the parts hang together, that it's been thought about, and that I can trust. This helps. I appreciate it. |
OK, so I finally had a chance to look at this, and I must confess, I don't see any particular problem with the interaction between With that said, I grant that there might be a use case for a traversal that yields the directory entry twice, but you should actually be able to layer that on top of a normal traversal yourself. It's definitely a bit more work, but I'm OK with niche use cases requiring more work. I'm going to leave this open for now in case anyone wants to rigorously explore the design space here, but IMO, we're fine as is and can move forward. |
Thanks for checking into this. I learned a lot and will submit a new issue if I hit a wall. |
This is about issue #18 and PR #19 . Aside: the more I dug in, the more I learned, and I'm grateful to all involved for the work on walkdir.
@mcharsley, I played around with contents_first, and for the most part, liked it.
However, it doesn't seem to work with
filter_entry()
orskip_current_directory()
-- the yielded entries look to be skipping the remainder of the directory the skipped directory is in, as opposed to the skipped directory.Additionally, I was looking for an option which also included the initial directory.
I see @michaelsproul proposed something which yields both edges, and I played around with a modified version of that and seemed to be able to do what I was looking for.
I also played around with writing an iterator adapter which also works okay, but didn't feel quite right, as it didn't work to implement WalkDir, so
skip_current_directory
was outside of that.I'm torn between a simpler walkdir plus another crate for an adapter controlling when and how often a directory is listed, and an integrated version, making use of the stacks that are already there. I could see either way, either rolling back the contents_first commit or adding to it to list the directory twice and have it working with filtering.
The text was updated successfully, but these errors were encountered: