GeezerVison is showing github.com — add pathlib.Path.walk method · Issue #90385 · python/cpython
add pathlib.Path.walk method #90385Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2022-01-02.18:34:12.724>
labels = ['type-feature', 'library', '3.11']
title = 'add pathlib.Path.walk method'
updated_at = <Date 2022-01-08.12:58:28.255>
user = 'https://github.com/Ovsyanka83' bugs.python.org fields:
activity = <Date 2022-01-08.12:58:28.255>
actor = 'Ovsyanka'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2022-01-02.18:34:12.724>
creator = 'Ovsyanka'
dependencies = []
files = []
hgrepos = []
issue_num = 46227
keywords = ['patch']
message_count = 4.0
messages = ['409511', '409514', '409994', '410098']
nosy_count = 4.0
nosy_names = ['eric.araujo', 'barneygale', 'ajoino', 'Ovsyanka']
pr_nums = ['30340']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue46227'
versions = ['Python 3.11'] Reacted by Tushar Sadhwani ActivityOvsyanka83 commented on Jan 2, 2022 MannequinAuthor More actions Pathlib is great, yet every time I have to parse a bunch of files, I have to use os.walk and join paths by hand. That's not a lot of code but I feel like pathlib should have higher-level abstractions for all path-related functionality of os. I propose we add a Path.walk method that could look like this:
def walk(self, topdown=True, onerror=None, followlinks=False):
for root, dirs, files in self._accessor.walk(
self,
topdown=topdown,
onerror=onerror,
followlinks=followlinks
):
root_path = Path(root)
yield (
root_path,
[root_path._make_child_relpath(dir_) for dir_ in dirs],
[root_path._make_child_relpath(file) for file in files],
)Note: this version does not handle a situation when top does not exist (similar to os.walk that also doesn't handle it and just returns an empty generator) Ovsyanka83 commented on Jan 2, 2022 MannequinAuthor More actions Some people could suggest using Path.glob instead but I found it to be less convenient for some use cases and generally slower (~2.7 times slower).
>>> timeit("list(Path('Lib').walk())", number=100, globals=globals())
1.9074640140170231
>>> timeit("list(Path('Lib').glob('**/*'))", number=100, globals=globals())
5.14890358998673merwok commented on Jan 7, 2022 The idea is interesting, and I agree that glob with a maxi wildcard is not a great solution. There is discussion on the PR about adding walk vs extending iterdir; could you post a message on discuss.python.org and sum up the the discussion? (Pull requests on the CPython repo are only used to discuss implementation, not for debating ideas or proposing features.) Ovsyanka83 commented on Jan 8, 2022 MannequinAuthor More actions zmievsa commented on May 8, 2022 The discussion ended on the following decisions:
- The original solution was based on wrapping os.walk but we have decided that it will be both faster and better in the long term to reimplement it (though I was not able to come up with any implementation that is significantly better than that of os.walk so it's 99% the same)
- The original solution was yielding a tuple[Path, list[Path], list[Path]] but we decided to stay true to the original implementation and yield tuple[Path, list[str], list[str]]. It is both better in terms of optimization and should cause a little less confusion to the users
- The original solution had the same arguments as os.walk but the interface was too convoluted so: we decided to rename arguments to snake-case and I decided to remove topdown argument by splitting walk into two methods. So we get walk() and walk_bottom_up(). I.e. Now we get two methods instead of one but with two modes of operation. The naming of methods is up to discussion because I myself am not very happy with these names but those are the best I found
I have been absent for quite some time so the pull request got stale and outdated so I'm opening a new one. brettcannon commented on Jul 8, 2022 The views on the change seem generally positive and the API seems like it makes sense, so I'm inclined to accept the change once the PR passes review. Reacted by Stanislav Zmiev and Tushar Sadhwani added 2 commits that reference this issue on Jul 23, 2022pythongh-90385: Add skip_unless_symlink decorator to test_walk_symlin… gh-90385: Add skip_unless_symlink decorator to test_walk_symlink_loca… zmievsa commented on Jul 24, 2022 @tiran I apologize for introducing this problem and I am really thankful to you for resolving it, especially so quickly! merwok commented on Jul 24, 2022 No worry! We all forgot to run the PR through buildbots before merging, so let’s just do that next time. Reacted by Stanislav Zmiev brettcannon commented on Jul 25, 2022 It's also partially my fault by forgetting that WASI doesn't support symlinks and thus to have you add the test guard for those sorts of platforms. Reacted by Stanislav Zmiev zmievsa commented on Jul 26, 2022 By the way, are we adding this PR into what's new? Or is it only gonna be a part of changelog? Seems big enough for what's new brettcannon commented on Jul 26, 2022 @Ovsyanka83 it should end up in What's New. If you would like to write a PR for that I will happily review it. We also go through the changelog before release to make sure everything relevant ends up in there, so we don't have to worry about it being missed. Reacted by Stanislav Zmiev
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|