Skip to content

Warn that rmdir() will stop removing non-empty directories - #565

Open
itzzdev09 wants to merge 2 commits into
fsspec:mainfrom
itzzdev09:fix/rmdir-future-warning
Open

itzzdev09 wants to merge 2 commits into
fsspec:mainfrom
itzzdev09:fix/rmdir-future-warning

Conversation

@itzzdev09

Copy link
Copy Markdown

Towards #559.

UPath.rmdir() removes a non-empty directory and everything in it, while PosixUPath/WindowsUPath and a wrapped pathlib.Path raise OSError, because they go through pathlib.Path.rmdir(). Same call, same local directory, two outcomes depending on how the path was spelled:

UPath("test").rmdir()          # OSError: Directory not empty
UPath("file://test").rmdir()   # removes test/ and its contents

As suggested in the issue, this does not change behaviour yet. recursive now defaults to UNSET_DEFAULT, and when it is not given and the directory is not empty, rmdir() warns:

UPath.rmdir() currently removes a non-empty directory and its contents. In universal-pathlib 0.4.0 it will raise, like pathlib.Path.rmdir() does. Pass recursive=True to keep removing the contents.

Passing recursive=True or recursive=False explicitly is unchanged and silent, so callers can opt out of the warning now and keep working after the default flips. The pathlib-backed classes already behave the way 0.4.0 will, so they do not warn.

One small fix comes along: the emptiness check was next(self.iterdir()), which raised StopIteration on an empty directory instead of falling through. It is now next(self.iterdir(), None).

Tests

test_rmdir_not_empty_warns_about_the_future_default and test_rmdir_not_empty_recursive_does_not_warn in the shared BaseTests, so every implementation is covered, with an override in TestProxyPathlibPath where the wrapped pathlib.Path raises instead.

Local suite: 2443 passed. test_pathlib_backport.py::test_signature_glob_pathlike[memory] fails on main here too, and the hdfs errors are a missing optional dependency.

Not in this PR

rmdir(recursive=False) on an empty directory calls fs.rm(..., recursive=False), which fsspec's LocalFileSystem rejects with ValueError: Cannot delete directory, set recursive=True. That is separate from the default change, so I left it alone; happy to follow up if you want it fixed before 0.4.0, since it becomes the common path once the default flips.

itzzdev09 and others added 2 commits September 14, 2026 23:29
…ted in HfPath.iterdir

Newer huggingface_hub releases raise ValueError instead of
NotImplementedError when asked to list a namespace such as a user's
repositories, which made test_iterdir_parent_iteration fail.

Fixes fsspec#562

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant