Skip to content

gh-90385: Add pathlib.Path.walk() method #92517

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

Merged

Conversation

zmievsa
Copy link
Contributor

@zmievsa zmievsa commented May 8, 2022

Automerge-Triggered-By: GH:brettcannon

@zmievsa zmievsa requested a review from brettcannon as a code owner May 8, 2022 22:47
@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

@ghost
Copy link

ghost commented May 9, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

@barneygale
Copy link
Contributor

A quick recipe that puts symlinks to directories in dirnames but doesn't walk into them:

for root, dirnames, filenames in Path().walk(follow_symlinks=True):
    # (do things with dirnames and filenames here)

    # don't walk into symlinks
    dirnames[:] = [name for name in dirnames if not (root / name).is_symlink())

Or alternatively:

for root, dirnames, filenames in Path().walk(follow_symlinks=False):
    for name in dirnames + filenames:
        path = root / name
        if path.is_dir():
            pass # do things to directory, or symlink to directory.

@brettcannon
Copy link
Member

Why walk_bottom_up() and not top_down or some such keyword parameter? I didn't see a discussion on the issue about this. The topdown parameter for os.walk() has worked out fine.

@zmievsa
Copy link
Contributor Author

zmievsa commented May 10, 2022

@brettcannon we have had a bit of a discussion here. For the purposes of saving your time, I'll lay down my logic here:

  • There are many complaints about os.walk having an overwhelming API so I tried to simplify it a bit
  • As a result, I split a single interface that could function in two modes with slightly different semantics (for example, allowing vs disallowing the modification of dirnames) into two interfaces (walk and walk_bottom_up), each of which has a single mode of functioning
  • The naming I used is quite clunky. Its purpose is basically: Path.walk is the default and most common use case which is why it has a shorter and non-symmetric name and Path.walk_bottom_up for other use cases
  • The more I work on this, the more I realize that os.walk had an overwhelming API because it did a complex task, not because it was written poorly or in a non-pythonic manner. So I'm completely open to any suggestions about refactoring my current API to make it clearer for the end users

@barneygale
Copy link
Contributor

Personally I reckon a top_down argument is better than splitting into two methods, but I can see what you were trying to do there.

@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

Copy link
Contributor

@barneygale barneygale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behaviour + implementation looks good to me! Still needs tests

@zmievsa zmievsa force-pushed the bpo-46227/add-pathlib.Path.walk-method branch from 2be287a to 203ec3d Compare July 17, 2022 20:10
@zmievsa
Copy link
Contributor Author

zmievsa commented Jul 17, 2022

I understand the difference between the keys. I tried to redo the commit with --amend (it helped in similar cases before) but reset --hard ultimately was what solved the problem

@zmievsa
Copy link
Contributor Author

zmievsa commented Jul 17, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was unfortunately a misunderstanding of what I was asking for. Luckily it's a minor thing so I can make the changes myself and then merge the PR once the tests pass.

@bedevere-bot

This comment was marked as outdated.

@brettcannon brettcannon self-requested a review July 22, 2022 23:46
@brettcannon brettcannon changed the title gh-90385: Add Path.walk method gh-90385: Add pathlib.Path.walk() method Jul 22, 2022
@brettcannon brettcannon added type-feature A feature request or enhancement 🤖 automerge topic-pathlib labels Jul 22, 2022
@miss-islington
Copy link
Contributor

Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit c1e9298 into python:main Jul 22, 2022
@brettcannon
Copy link
Member

@Ovsyanka83 thanks so much!

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot wasm32-wasi 3.x has failed when building commit c1e9298.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1046/builds/244) and take a look at the build logs.
  4. Check if the failure is related to this commit (c1e9298) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1046/builds/244

Failed tests:

  • test_pathlib

Failed subtests:

  • test_walk_symlink_location - test.test_pathlib.WalkTests.test_walk_symlink_location

Summary of the results of the build (if available):

== Tests result: FAILURE ==

328 tests OK.

10 slowest tests:

  • test_tokenize: 48.5 sec
  • test_unparse: 31.6 sec
  • test_lib2to3: 30.1 sec
  • test_capi: 22.2 sec
  • test_unicodedata: 14.7 sec
  • test_pickle: 12.8 sec
  • test_decimal: 12.5 sec
  • test_statistics: 10.2 sec
  • test_buffer: 7.2 sec
  • test_compile: 5.7 sec

1 test failed:
test_pathlib

107 tests skipped:
test__xxsubinterpreters test_asyncgen test_asynchat test_asyncio
test_asyncore test_bz2 test_check_c_globals test_clinic
test_cmd_line test_concurrent_futures test_contextlib_async
test_ctypes test_curses test_dbm_gnu test_dbm_ndbm test_devpoll
test_doctest test_docxmlrpc test_dtrace test_embed test_epoll
test_faulthandler test_fcntl test_file_eintr test_fork1
test_ftplib test_gdb test_grp test_gzip test_httplib
test_httpservers test_idle test_imaplib test_interpreters
test_ioctl test_kqueue test_launcher test_lzma test_mailbox
test_mmap test_msilib test_multiprocessing_fork
test_multiprocessing_forkserver test_multiprocessing_main_handling
test_multiprocessing_spawn test_nis test_openpty test_ossaudiodev
test_pdb test_pipes test_poll test_poplib test_pty test_pwd
test_queue test_readline test_regrtest test_repl test_resource
test_select test_selectors test_smtpd test_smtplib test_smtpnet
test_socket test_socketserver test_spwd test_sqlite3 test_ssl
test_stable_abi_ctypes test_startfile test_subprocess
test_sys_settrace test_syslog test_tcl test_telnetlib test_thread
test_threadedtempfile test_threading test_threading_local test_tix
test_tkinter test_tools test_ttk test_ttk_textonly test_turtle
test_urllib test_urllib2 test_urllib2_localnet test_urllib2net
test_urllib_response test_urllibnet test_venv test_wait3
test_wait4 test_webbrowser test_winconsoleio test_winreg
test_winsound test_wsgiref test_xmlrpc test_xmlrpc_net
test_xxlimited test_zipfile64 test_zipimport_support test_zlib
test_zoneinfo
0:03:49 load avg: 1.95
0:03:49 load avg: 1.95 Re-running failed tests is not supported with --python host runner option.

Total duration: 3 min 49 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/Lib/test/test_pathlib.py", line 2626, in test_walk_symlink_location
    self.assertIn("link", files)
AssertionError: 'link' not found in ['tmp3']

@python python deleted a comment from bedevere-bot Jul 24, 2022
@zmievsa
Copy link
Contributor Author

zmievsa commented Jul 24, 2022

I'm on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-pathlib type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants