ncoghlan / walkdir

Other
11 stars 3 forks source link

changed all_paths, and dir_paths to work with min/max depth #5

Closed palaviv closed 8 years ago

palaviv commented 8 years ago

As report in Issue #4 all_paths and dir_paths don't yield all expected path's when depth is specified. I changed all_paths and dir_paths to yield subdirs so even when min/max depth is specified we will receive all expected paths.

There are two issues with the changes to all_paths and dir_paths:

  1. The implementation rely on that os.walk will be DFS.
  2. The order of the path's yielding is changed as followed: Lets assume the following directory tree:
test
|--file1.txt
|--file2.txt
|--test2
\----file1.txt
\----file2.txt
\----test3
|--test4
\----file1.txt
\----test5

For all_paths(filtered_walk('test')) -> [test, file1.txt, file2.txt, test2, test4, test2/file1.txt, test2/file2, test2/test3, test4/file1.txt, test4/test5] For all_paths(filtered_walk('test', min_depth=1)) -> [test2, test2/file1.txt, test2/file2, test2/test3, , test4, test4/file1.txt, test4/test5]

As you can see when min_depth there is inconsistency in the order as previously.

landscape-bot commented 8 years ago

Code Health Repository health increased by 2% when pulling 035718c on palaviv:all_paths-work-with-max/min-depth into 668dae2 on ncoghlan:master.

ncoghlan commented 8 years ago

I didn't fully understand your observation above until reviewing the PR. Now that I do understand it, I think the key points to note in the documentation are:

The output discrepancy then comes from the fact that in min_depth=1, each subdirectory is listed as the root of its own tree rather than as a subdirectory of the overall root.

I also filed #6 to note that some of the APIs currently won't work with os.fwalk() (since that produces a 4-tuple instead of a 3-tuple), and #7 to note that the docs could stand to be clearer on which functions rely on a top-down(/breadth-first) walk in order to implement directory traversal filtering

landscape-bot commented 8 years ago

Code Health Repository health decreased by 8% when pulling 4bbffc2 on palaviv:all_paths-work-with-max/min-depth into 668dae2 on ncoghlan:master.

landscape-bot commented 8 years ago

Code Health Repository health increased by 2% when pulling c5d9bfc on palaviv:all_paths-work-with-max/min-depth into 668dae2 on ncoghlan:master.

palaviv commented 8 years ago

I improved the documentation of this PR. I will work on #6, #7 on a different pull requests. In addition i think this supersedes #2 as well.

ncoghlan commented 8 years ago

Thanks Aviv, this looks great to me - my only comments are for a few typos in the new docs and docstring updates.

landscape-bot commented 8 years ago

Code Health Repository health increased by 2% when pulling ff957f5 on palaviv:all_paths-work-with-max/min-depth into 668dae2 on ncoghlan:master.

palaviv commented 8 years ago

I fixed the typos. After merging this I will open a pull request with the tests from #2.

ncoghlan commented 8 years ago

Thank you!