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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions Doc/library/pathlib.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1637,20 +1637,11 @@ Copying, renaming and deleting
:meth:`Path.delete` to remove a non-empty directory.


.. method:: Path.delete(ignore_errors=False, on_error=None)
.. method:: Path.delete()

Delete this file or directory. If this path refers to a non-empty
directory, its files and sub-directories are deleted recursively.

If *ignore_errors* is true, errors resulting from failed deletions will be
ignored. If *ignore_errors* is false or omitted, and a callable is given as
the optional *on_error* argument, it will be called with one argument of
type :exc:`OSError` each time an exception is raised. The callable can
handle the error to continue the deletion process or re-raise it to stop.
Note that the filename is available as the :attr:`~OSError.filename`
attribute of the exception object. If neither *ignore_errors* nor
*on_error* are supplied, exceptions are propagated to the caller.

.. note::

When deleting non-empty directories on platforms that lack the necessary
Expand Down
23 changes: 4 additions & 19 deletions Lib/pathlib/_abc.py
Original file line number Diff line number Diff line change
Expand Up @@ -923,23 +923,13 @@ def rmdir(self):
"""
raise UnsupportedOperation(self._unsupported_msg('rmdir()'))

def delete(self, ignore_errors=False, on_error=None):
def delete(self):
"""
Delete this file or directory (including all sub-directories).

If *ignore_errors* is true, exceptions raised from scanning the
filesystem and removing files and directories are ignored. Otherwise,
if *on_error* is set, it will be called to handle the error. If
neither *ignore_errors* nor *on_error* are set, exceptions are
propagated to the caller.
"""
if ignore_errors:
def on_error(err):
pass
elif on_error is None:
if self.is_dir(follow_symlinks=False):
def on_error(err):
raise err
if self.is_dir(follow_symlinks=False):
results = self.walk(
on_error=on_error,
top_down=False, # So we rmdir() empty directories.
Expand All @@ -955,14 +945,9 @@ def on_error(err):
dirpath.joinpath(name).rmdir()
except OSError as err:
on_error(err)
delete_self = self.rmdir
self.rmdir()
else:
delete_self = self.unlink
try:
delete_self()
except OSError as err:
err.filename = str(self)
on_error(err)
self.unlink()
delete.avoids_symlink_attacks = False

def owner(self, *, follow_symlinks=True):
Expand Down
30 changes: 7 additions & 23 deletions Lib/pathlib/_local.py
Original file line number Diff line number Diff line change
Expand Up @@ -824,32 +824,16 @@ def rmdir(self):
"""
os.rmdir(self)

def delete(self, ignore_errors=False, on_error=None):
def delete(self):
"""
Delete this file or directory (including all sub-directories).

If *ignore_errors* is true, exceptions raised from scanning the
filesystem and removing files and directories are ignored. Otherwise,
if *on_error* is set, it will be called to handle the error. If
neither *ignore_errors* nor *on_error* are set, exceptions are
propagated to the caller.
"""
if self.is_dir(follow_symlinks=False):
onexc = None
if on_error:
def onexc(func, filename, err):
err.filename = filename
on_error(err)
shutil.rmtree(str(self), ignore_errors, onexc=onexc)
"""
if self.is_symlink() or self.is_junction():
self.unlink()
elif self.is_dir():
shutil.rmtree(str(self))
else:
try:
self.unlink()
except OSError as err:
if not ignore_errors:
if on_error:
on_error(err)
else:
raise
self.unlink()

delete.avoids_symlink_attacks = shutil.rmtree.avoids_symlink_attacks

Expand Down
123 changes: 11 additions & 112 deletions Lib/test/test_pathlib/test_pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -715,11 +715,6 @@ def test_copy_dir_no_read_permission(self):
target = base / 'copyE'
self.assertRaises(PermissionError, source.copy, target)
self.assertFalse(target.exists())
errors = []
source.copy(target, on_error=errors.append)
self.assertEqual(len(errors), 1)
self.assertIsInstance(errors[0], PermissionError)
self.assertFalse(target.exists())

def test_copy_dir_preserve_metadata(self):
base = self.cls(self.base)
Expand Down Expand Up @@ -904,10 +899,7 @@ def test_delete_unwritable(self):
child_dir_path.chmod(new_mode)
tmp.chmod(new_mode)

errors = []
tmp.delete(on_error=errors.append)
# Test whether onerror has actually been called.
self.assertEqual(len(errors), 3)
self.assertRaises(PermissionError, tmp.delete)
finally:
tmp.chmod(old_dir_mode)
child_file_path.chmod(old_child_file_mode)
Expand Down Expand Up @@ -942,42 +934,16 @@ def test_delete_outer_junction(self):
import _winapi
tmp = self.cls(self.base, 'delete')
tmp.mkdir()
try:
src = tmp / 'cheese'
dst = tmp / 'shop'
src.mkdir()
spam = src / 'spam'
spam.write_text('')
_winapi.CreateJunction(str(src), str(dst))
self.assertRaises(OSError, dst.delete)
dst.delete(ignore_errors=True)
finally:
tmp.delete(ignore_errors=True)

@needs_windows
def test_delete_outer_junction_on_error(self):
import _winapi
tmp = self.cls(self.base, 'delete')
tmp.mkdir()
dir_ = tmp / 'dir'
dir_.mkdir()
link = tmp / 'link'
_winapi.CreateJunction(str(dir_), str(link))
try:
self.assertRaises(OSError, link.delete)
self.assertTrue(dir_.exists())
self.assertTrue(link.exists(follow_symlinks=False))
errors = []

def on_error(error):
errors.append(error)

link.delete(on_error=on_error)
self.assertEqual(len(errors), 1)
self.assertIsInstance(errors[0], OSError)
self.assertEqual(errors[0].filename, str(link))
finally:
os.unlink(str(link))
src = tmp / 'cheese'
dst = tmp / 'shop'
src.mkdir()
spam = src / 'spam'
spam.write_text('')
_winapi.CreateJunction(str(src), str(dst))
dst.delete()
self.assertFalse(dst.exists())
self.assertTrue(spam.exists())
self.assertTrue(src.exists())

@unittest.skipUnless(delete_use_fd_functions, "requires safe delete")
def test_delete_fails_on_close(self):
Expand All @@ -1003,16 +969,6 @@ def close(fd):
self.assertTrue(dir2.is_dir())
self.assertEqual(close_count, 2)

close_count = 0
errors = []

with swap_attr(os, 'close', close) as orig_close:
dir1.delete(on_error=errors.append)
self.assertEqual(len(errors), 2)
self.assertEqual(errors[0].filename, str(dir2))
self.assertEqual(errors[1].filename, str(dir1))
self.assertEqual(close_count, 2)

@unittest.skipUnless(hasattr(os, "mkfifo"), 'requires os.mkfifo()')
@unittest.skipIf(sys.platform == "vxworks",
"fifo requires special path on VxWorks")
Expand All @@ -1028,63 +984,6 @@ def test_delete_on_named_pipe(self):
p.delete()
self.assertFalse(p.exists())

@unittest.skipIf(sys.platform[:6] == 'cygwin',
"This test can't be run on Cygwin (issue #1071513).")
@os_helper.skip_if_dac_override
@os_helper.skip_unless_working_chmod
def test_delete_deleted_race_condition(self):
# bpo-37260
#
# Test that a file or a directory deleted after it is enumerated
# by scandir() but before unlink() or rmdr() is called doesn't
# generate any errors.
def on_error(exc):
assert exc.filename
if not isinstance(exc, PermissionError):
raise
# Make the parent and the children writeable.
for p, mode in zip(paths, old_modes):
p.chmod(mode)
# Remove other dirs except one.
keep = next(p for p in dirs if str(p) != exc.filename)
for p in dirs:
if p != keep:
p.rmdir()
# Remove other files except one.
keep = next(p for p in files if str(p) != exc.filename)
for p in files:
if p != keep:
p.unlink()

tmp = self.cls(self.base, 'delete')
tmp.mkdir()
paths = [tmp] + [tmp / f'child{i}' for i in range(6)]
dirs = paths[1::2]
files = paths[2::2]
for path in dirs:
path.mkdir()
for path in files:
path.write_text('')

old_modes = [path.stat().st_mode for path in paths]

# Make the parent and the children non-writeable.
new_mode = stat.S_IREAD | stat.S_IEXEC
for path in reversed(paths):
path.chmod(new_mode)

try:
tmp.delete(on_error=on_error)
except:
# Test failed, so cleanup artifacts.
for path, mode in zip(paths, old_modes):
try:
path.chmod(mode)
except OSError:
pass
tmp.delete()
raise

def test_delete_does_not_choke_on_failing_lstat(self):
try:
orig_lstat = os.lstat
Expand Down
8 changes: 0 additions & 8 deletions Lib/test/test_pathlib/test_pathlib_abc.py
Original file line number Diff line number Diff line change
Expand Up @@ -2705,14 +2705,6 @@ def test_delete_missing(self):
# filename is guaranteed not to exist
filename = tmp / 'foo'
self.assertRaises(FileNotFoundError, filename.delete)
# test that ignore_errors option is honored
filename.delete(ignore_errors=True)
# test on_error
errors = []
filename.delete(on_error=errors.append)
self.assertEqual(len(errors), 1)
self.assertIsInstance(errors[0], FileNotFoundError)
self.assertEqual(errors[0].filename, str(filename))

def setUpWalk(self):
# Build:
Expand Down
Loading