GeezerVison is showing github.com — GH-73991: Prune `pathlib.Path.delete()` arguments by barneygale · Pull Request #123158 · python/cpython
Conversation
barneygale
commented
Aug 19, 2024
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Remove the ignore_errors and on_error arguments from Path.delete(). This functionality was carried over from shutil.rmtree(), but its design needs to be re-evaluated in its new context. Some possible alternate designs:
- We may wish to support a missing_ok argument (like
Path.unlink())
- Or automatically
chmod() and retry operations when we hit a permission error (like tempfile.TemporaryDirectory)
- Or retry operations with a backoff (like
test.support.os_helper.rmtree()),
- Or utilise exception groups
It's best to leave our options open for now. I don't want to risk these arguments landing in 3.14 final because I got hit by a bus or something.
No news because Path.delete() remains unreleased.
📚 Documentation preview 📚: https://cpython-previews--123158.org.readthedocs.build/
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Remove the *ignore_errors* and *on_error* arguments from `Path.delete()`.
This functionality was carried over from `shutil`, but its design needs to
be re-considered in its new context. For example, we may wish to support a
*missing_ok* argument (like `Path.unlink()`), or automatically `chmod()`
and retry operations when we hit a permission error (like
`tempfile.TemporaryDirectory`), or retry operations with a backoff (like
`test.support.os_helper.rmtree()`), or utilise exception groups, etc. It's
best to leave our options open for now.
barneygale
changed the title
GH-73991: Prune pathlib.Path.delete()
GH-73991: Prune pathlib.Path.delete() arguments
Aug 21, 2024
|
I can understand the logic that we omit the arguments until we're sure we have the right design, but in my experience, a directory tree delete very often needs these arguments (specifically on_error)1. Finding a read-only file partway through a directory deletion can cause the deletion to fail half-done, leaving the filesystem in a mess. And having to traverse the directory tree, either in advance or after the fact, to locate the offending file and fix the issue, can be extremely costly in a large directory structure.
To put this another way, the key benefit of on_error is to allow problems to be handled in context. Without that capability, I think I'd always advise using shutil.rmtree over Path.delete.
I'd rather defer making Path.delete public until we have the error handling sorted out, over releasing it in a state that makes it strictly worse than shutil.rmtree.
Footnotes
-
For example, I thought that the way .git directories were created on Windows used to mean that they would always cause failures without corrective action in on_error, but when I tested just now, it looks like things might have changed there. Nevertheless, the point about fixing errors in context remains. ↩
|
barneygale reacted with heart emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Thanks very much, that's most helpful. I'll close this PR for now, and open a new topic on the forum (in a few weeks time) about what to do with these arguments. I'll try to make the case that the number of practical on_error implementations is limited - in general you retry the unlink() or rmdir(), possibly after a chmod() and/or a time delay - and that there might be a more pathlib-y way to expose these options.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
barneygale
added a commit
to barneygale/cpython
that referenced
this pull request
Aug 25, 2024
Per feedback from Paul Moore on pythonGH-123158, it's better to defer making
`Path.delete()` public than ship it with under-designed error handling
capabilities.
We leave a remnant `_delete()` method, which is used by `move()`. Any
functionality not needed by `move()` is deleted.
barneygale
added a commit
that referenced
this pull request
Aug 26, 2024
Per feedback from Paul Moore on GH-123158, it's better to defer making
`Path.delete()` public than ship it with under-designed error handling
capabilities.
We leave a remnant `_delete()` method, which is used by `move()`. Any
functionality not needed by `move()` is deleted.
|
A tiny point, somewhat after the event. In the current shutil.rmtree the onerror callback has been deprecated in favour of a similar but different onexc. Same arguments apply as above, and I rather agree with @pfmoore ("the key benefit of on_error is to allow problems to be handled in context").
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
pfmoore
Awaiting requested review from pfmoore
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|