Skip to content

GH-73991: Prune pathlib.Path.delete() arguments #123158

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

Closed
wants to merge 5 commits into from

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Aug 19, 2024

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/

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 barneygale marked this pull request as ready for review August 20, 2024 21:56
@barneygale barneygale requested a review from pfmoore August 20, 2024 21:58
@barneygale barneygale changed the title GH-73991: Prune pathlib.Path.delete() GH-73991: Prune pathlib.Path.delete() arguments Aug 21, 2024
@pfmoore
Copy link
Member

pfmoore commented Aug 22, 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

  1. 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
Copy link
Contributor Author

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.

@barneygale barneygale closed this Aug 22, 2024
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.
@tjg-global
Copy link

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").

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.

3 participants