Skip to content

gh-89727: Fix pathlib.Path.walk RecursionError on deep trees#100282

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.

Sign up for GitHub

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

Jump to bottom
Merged
barneygale merged 20 commits into from
Mar 22, 2023
Merged

gh-89727: Fix pathlib.Path.walk RecursionError on deep trees #100282

barneygale merged 20 commits into from
Mar 22, 2023

Conversation

@zmievsa
Copy link
Contributor

@zmievsazmievsa commented Dec 15, 2022•
edited
Loading

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

@bedevere-botbedevere-bot mentioned this pull request Dec 15, 2022
Open
@netlify
Copy link

netlifybot commented Dec 15, 2022•
edited
Loading

✅ Deploy Preview for python-cpython-preview canceled.

NameLink
🔨 Latest commit2bfc47d
🔍 Latest deploy loghttps://app.netlify.com/sites/python-cpython-preview/deploys/639c50140be628000884140d

@zmievsa
Copy link
ContributorAuthor

zmievsa commented Dec 15, 2022•
edited
Loading

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!

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

brettcannon commented Dec 16, 2022

/cc @barneygale

kalvdans
kalvdans reviewed Dec 20, 2022
View reviewed changes
carljm
carljm reviewed Dec 20, 2022
View reviewed changes
carljm
carljm approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@carljmcarljm 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
ContributorAuthor

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
ContributorAuthor

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

barneygale commented Dec 24, 2022

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

barneygale
barneygale reviewed Jan 2, 2023
View reviewed changes
@Ovsyanka83@barneygale
Update Lib/pathlib.py
b7d36de
Co-authored-by: Barney Gale <barney.gale@gmail.com>
barneygale
barneygale approved these changes Jan 2, 2023
View reviewed changes
Copy link
Contributor

@barneygalebarneygale 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
ContributorAuthor

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

AlexWaygood commented Jan 11, 2023

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

brettcannon
brettcannon approved these changes Jan 21, 2023
View reviewed changes
@Ovsyanka83@brettcannon
Apply suggestions from code review
49d4237
Co-authored-by: Brett Cannon <brett@python.org>
@barneygale
Copy link
Contributor

barneygale commented Feb 1, 2023•
edited
Loading

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

CuriousLearner
CuriousLearner approved these changes Feb 18, 2023
View reviewed changes
Copy link
Member

@CuriousLearnerCuriousLearner 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

barneygale commented Feb 28, 2023

@AlexWaygoodhttps://youtu.be/CdqoNKCCt7A

barneygale
barneygale reviewed Mar 11, 2023
View reviewed changes
@barneygalebarneygale mentioned this pull request Mar 12, 2023
Closed
@ionite34ionite34 mentioned this pull request Mar 12, 2023
Closed
@barneygalebarneygale merged commit 713df2c into python:mainMar 22, 2023
@zmievsa
Copy link
ContributorAuthor

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?

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

AlexWaygood commented Mar 22, 2023

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

@barneygale
Copy link
Contributor

barneygale commented Mar 22, 2023

@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
@zmievsa@barneygale
@brettcannon@Fidget-Spinner
pythonGH-89727: Fix pathlib.Path.walk RecursionError on deep trees (p…
b645f1b
…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
@zmievsa@barneygale
@brettcannon@warsaw
pythonGH-89727: Fix pathlib.Path.walk RecursionError on deep trees (p…
dfeb972
…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
@barneygale
pythonGH-102613: Fix recursion error frompathlib.Path.glob()
71dc988
Use `Path.walk()` to implement the recursive wildcard `**`. This method uses an iterative (rather than recursive) walk - see pythonGH-100282.
@barneygalebarneygale mentioned this pull request May 11, 2023
Merged
barneygale added a commit that referenced this pull request May 15, 2023
@barneygale
GH-102613: Fix recursion error frompathlib.Path.glob()(GH-104373)
cb88ae6
Use `Path.walk()` to implement the recursive wildcard `**`. This method uses an iterative (rather than recursive) walk - see GH-100282.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannonbrettcannonbrettcannon approved these changes

@carljmcarljmcarljm approved these changes

@barneygalebarneygalebarneygale approved these changes

@CuriousLearnerCuriousLearnerCuriousLearner approved these changes

@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka

@AlexWaygoodAlexWaygoodAwaiting requested review from AlexWaygood

+1 more reviewer

@kalvdanskalvdanskalvdans left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

topic-pathlib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@zmievsa@brettcannon@barneygale@AlexWaygood@carljm@kalvdans@CuriousLearner@bedevere-bot@Ovsyanka83