> 1) Serhiy's point about not needing to build the symlinks set when followlinks is True is a good one, because it'll never get used. Just a "if not followlinks: ..." around that try/except would do it. Though this is a small optimization, as I expect 95% of people use os.walk() with followlinks=False, which is the default.
No, it's not a "small optimization". I/O are expensive, especially disk I/O. Try os.walk() on a NFS share with the cache disabled ;-)
> My first note was about efficiency of the implementation. When followlinks is
> true, you can avoid testing entry.is_link() and creating the symlinks set.
Oh, I misunderstood your comment. The changeset 1a972140ab62 avoids calling entry.is_symlink() when it's not needed. Thanks for the report.
--
walk_added_symlink_to_dir.patch: Small modification to os.walk() to keep the performances of os.scandir() *and* still supported adding symlinks to dirs (no backward compatibility changes).
Happy? :-)
I started to work on an unit test, but I don't understand how WalkTests and FwalkTests are written.
The setUp() method contais unit tests?! For example, setUp() has a unit test removing a directory from the dirs list to check os.walk() behaviour.
I will try harder to understand how these tests are written and post a patch for tests later. |