X Tutup
The Wayback Machine - https://web.archive.org/web/20221223144527/https://github.com/python/cpython/pull/92517
Skip to content
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

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

Merged

Conversation

Ovsyanka83
Copy link
Contributor

@Ovsyanka83 Ovsyanka83 commented May 8, 2022

Automerge-Triggered-By: GH:brettcannon

@Ovsyanka83 Ovsyanka83 requested a review from brettcannon as a code owner May 8, 2022
@bedevere-bot
Copy link

bedevere-bot commented May 8, 2022

Every change to Python requires a NEWS entry.

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

Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented May 9, 2022

Every change to Python requires a NEWS entry.

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

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented May 9, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

bedevere-bot commented May 9, 2022

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

bedevere-bot commented May 9, 2022

Every change to Python requires a NEWS entry.

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

Lib/pathlib.py Show resolved Hide resolved
@barneygale
Copy link
Contributor

barneygale commented May 9, 2022

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

brettcannon commented May 10, 2022

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.

@Ovsyanka83
Copy link
Contributor Author

Ovsyanka83 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

barneygale commented May 10, 2022

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

bedevere-bot commented May 10, 2022

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

bedevere-bot commented May 10, 2022

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

Behaviour + implementation looks good to me! Still needs tests

Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/pathlib.py Outdated Show resolved Hide resolved
@Ovsyanka83 Ovsyanka83 force-pushed the bpo-46227/add-pathlib.Path.walk-method branch from 2be287a to 203ec3d Compare Jul 17, 2022
@Ovsyanka83
Copy link
Contributor Author

Ovsyanka83 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

@Ovsyanka83
Copy link
Contributor Author

Ovsyanka83 commented Jul 17, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Jul 17, 2022

Thanks for making the requested changes!

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

Copy link
Member

@brettcannon brettcannon left a comment

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.

Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment was marked as outdated.

@brettcannon brettcannon self-requested a review Jul 22, 2022
@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 PR will be merged once it's been approved and all CI passed expert-pathlib labels Jul 22, 2022
@miss-islington
Copy link
Contributor

miss-islington commented Jul 22, 2022

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

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

brettcannon commented Jul 22, 2022

@Ovsyanka83 thanks so much!

@bedevere-bot
Copy link

bedevere-bot commented Jul 24, 2022

⚠️⚠️⚠️ 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
@Ovsyanka83
Copy link
Contributor Author

Ovsyanka83 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
expert-pathlib 🤖 automerge PR will be merged once it's been approved and all CI passed type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants
X Tutup