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-89727: Fix pathlib.Path.walk RecursionError on deep trees #100282

Merged

Conversation

zmievsa
Copy link
Contributor

@zmievsa zmievsa commented Dec 15, 2022

Use a stack to implement pathlib.Path.walk iteratively instead of recursively to avoid hitting recursion limits on deeply nested trees.

@netlify
Copy link

netlify bot commented Dec 15, 2022

Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit 2bfc47d
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/639c50140be628000884140d

@zmievsa
Copy link
Contributor Author

zmievsa commented Dec 15, 2022

TODO:

  • Add a news blip
  • Add a test that checks for deeply nested directories
  • Figure out a better name for is_result

Update:
All done!

@zmievsa zmievsa marked this pull request as ready for review December 16, 2022 10:32
@brettcannon
Copy link
Member

/cc @barneygale

Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

LGTM. I assume because of the initial underscore we don't have to worry about people who may have subclassed Path and overridden the _walk method (now to no effect.)

@zmievsa
Copy link
Contributor Author

zmievsa commented Dec 20, 2022

@carljm remember that Path.walk has only been created in 3.12 which means that 99% of libraries/projects do not and cannot depend on it yet.

@zmievsa
Copy link
Contributor Author

zmievsa commented Dec 24, 2022

@barneygale any estimates of when you will have time to take a look at it? No pressure or rush though.

@barneygale
Copy link
Contributor

I'll look at this within the next few days!

Lib/pathlib.py Outdated Show resolved Hide resolved
Co-authored-by: Barney Gale <barney.gale@gmail.com>
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.

Looks good to me!

@zmievsa
Copy link
Contributor Author

zmievsa commented Jan 11, 2023

@AlexWaygood what would be the next steps for this PR? I am ready to continue improving it

@AlexWaygood
Copy link
Member

Sorry @Ovsyanka83, I'll try to take a proper look soon!

Lib/pathlib.py Outdated Show resolved Hide resolved
Lib/test/test_pathlib.py Outdated Show resolved Hide resolved
Co-authored-by: Brett Cannon <brett@python.org>
@barneygale
Copy link
Contributor

barneygale commented Feb 1, 2023

@AlexWaygood gentle nudge to review this when you have the time! Thanks :)

Copy link
Member

@CuriousLearner CuriousLearner left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for your contribution.

@barneygale
Copy link
Contributor

Lib/pathlib.py Outdated Show resolved Hide resolved
@barneygale barneygale merged commit 713df2c into python:main Mar 22, 2023
@zmievsa
Copy link
Contributor Author

zmievsa commented Mar 22, 2023

@barneygale thanks!

Wow. Have you received merge rights? Congrats! Did they make you a core developer or is that a special rule just for you and pathlib?

@zmievsa zmievsa deleted the gh-89727/fix-pathlib.Path.walk-recursion-depth branch March 22, 2023 18:20
@AlexWaygood
Copy link
Member

We made him a core dev! https://discuss.python.org/t/vote-to-promote-barney-gale/24801

@barneygale
Copy link
Contributor

@Ovsyanka83 thanks for your patience and for working on this, it's a neat patch :). I'm hoping to build an iterative version of glob() atop!

Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
…ythonGH-100282)

Use a stack to implement `pathlib.Path.walk()` iteratively instead of recursively to avoid hitting recursion limits on deeply nested trees.

Co-authored-by: Barney Gale <barney.gale@gmail.com>
Co-authored-by: Brett Cannon <brett@python.org>
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
…ythonGH-100282)

Use a stack to implement `pathlib.Path.walk()` iteratively instead of recursively to avoid hitting recursion limits on deeply nested trees.

Co-authored-by: Barney Gale <barney.gale@gmail.com>
Co-authored-by: Brett Cannon <brett@python.org>
barneygale added a commit to barneygale/cpython that referenced this pull request May 11, 2023
Use `Path.walk()` to implement the recursive wildcard `**`. This method
uses an iterative (rather than recursive) walk - see pythonGH-100282.
barneygale added a commit that referenced this pull request May 15, 2023
Use `Path.walk()` to implement the recursive wildcard `**`. This method
uses an iterative (rather than recursive) walk - see GH-100282.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants