GeezerVison

to do:

• clean up code
• add some tests

The BytesWalkTests.walk and BytesFwalkTests.walk wrapper functions actually prevent late modification (after iteration of dirs has started) because of the encoding/decoding logic they use which sets dirs before iterating over it. These are of course only for testing purposes, but it means that if we want to run test_walk_late_dirs_modification in them as well as in WalkTests, we'd need an alternate walk wrapper which this test uses and some logic to handle the bytes results in the Bytes... classes - maybe not worth it?

A few more notes for context:

The first commit has more minimal changes similar to this suggestion. In that commit, because we're using an iterator, the implementation is a bit awkward and inefficient (i.e. only ever calling next(dirs) once per loop). There's two cases in which we ignore the value given by next(dirs) and move on:

1. With topdown=True, the followlinks or not islink(top) check fails
2. The line scandir_it = scandir(top) throws an OSError which is caught (and not re-raised by onerror)
   The subsequent changes handle these more naturally by looping through the iterator instead of calling continue to go through the outer loop again.

The subsequent changes also separate the top down and bottom up logic further (even before splitting into separate functions). Note that even before this PR, there was a fair amount of code that only applied when we have topdown=False:

• The if isinstance(top, tuple): call at the top of the outer loop
• walk_dirs creation and if isinstance(top, tuple): inside the scandir_it loop
• if topdown: block at the bottom

Do we really want to introduce this much complexity for a completely undocumented and possibly unused feature?

Do we really want to introduce this much complexity for a completely undocumented and possibly unused feature?

Maybe not. I made this PR to see what it would look like, but am not attached to it. If I were designing os.walk from scratch, I wouldn't support this late editing of dirs - at least not in this way where the list can be edited during its iteration. I think this could be reasonably considered behavior that produces an undefined result, or simply considered a bug, although it was never documented as such.

But I strongly suspect it is used out there (maybe even by accident), and consistent behavior is valuable. If we don't support this, it'd be nice to have some sort of warning about the behavior change but I also don't see a reasonable way to do that. (Checking if dirs was modified would be complex or expensive too and kind of weird).

I'd suggest at least noting the change in behavior in release notes if we don't support the old behavior. I also do prefer separating the top down and bottom up logic as in this PR, although that could be a separate matter.

Other considerations:

• Complexity (or at least code change) is a bit less in the first commit here if that seems any better.
• I don't like the idea of copying this mess over to pathlib.Path.walk, and I also don't like the idea of having different behavior, especially if this is considered a bug or undefined behavior. It's also useful to keep the code similar between the two for maintenance, understanding logic, future changes, etc

I don't like the idea of copying this mess over to pathlib.Path.walk, and I also don't like the idea of having different behavior, especially if this is considered a bug or undefined behavior. It's also useful to keep the code similar between the two for maintenance, understanding logic, future changes, etc

pathlib's version of walk() was merged only because it improved the ergonomics of os.walk(), including handling symlinks to directories more consistently, which isn't possible in os.walk() for backwards compatibility.

Mutating dirnames after resuming the walk isn't included or even hinted at in the pathlib docs so I don't want to expend any effort supporting it.