I have looked through the history of shutil.rmtree up until the original contribution by Guido. A few notes:
- ignore_errors argument seems entirely unnecessary as in os.walk it is successfully replaced by onerror. However, it feels like they kept it for backwards compatibility because it was available from the very first draft of shutil.rmtree
- shutil.rmtree is still worked on, as in the example with dir_fd (2 mo ago) by Serhiy Storchaka. It is also pretty darn complex and for a good reason
So I once again argue that if we include it in pathlib, we should wrap shutil. Question to @barneygale and all contributors into pathlib: why doesn’t pathlib use shutil at all? It seems so strange that there must be a good reason.
Either way, I’m gonna write up a pull request for addition of rmtree right now and we can continue from there.
+1. If/when we add an
AbstractPath class, we might want to add a generic implementation using
AbstractPath.walk() etc. But for now we can call straight through to
ignore_errors argument seems entirely unnecessary as in os.walk it is successfully replaced by onerror. However, it feels like they kept it for backwards compatibility because it was available from the very first draft of shutil.rmtree
ignore_errors=True is easier to write than
onerror=lambda a, b, c: pass?
If onerror is provided, it must be a callable that accepts three parameters: function, path, and excinfo.
This is different from the on_error function for your proposed
walk() method (which only accepts a single parameter) - any way we can reconcile the difference in pathlib?
ignore_errors=True is easier BUT in os.walk we do on_error=None.
Please, take a look at how I handled it in the pull request. I really like this implementation but as always, I’m open to change it.
Docs and tests are only a draft. I really want to add 2 more tests but I haven’t had time to figure out how to test partial removal just yet.
As for your question about onerror signature, we could wrap the user’s function to convert args as follows:
def _convert_err(func): @functools.wraps(func) def wrapper(exc_type, exc, tb): return func(exc) return wrapper
or just go with my current approach and ignore the inconsistency. Though I really don’t want to ignore it.
I messed up the format:
def _convert_err(func): @functools.wraps(func) def wrapper(func, path, exc_info): return func(exc_info) return wrapper
But it feels like they’re doing it like so for a good reason. It makes for a more precise error handling in more complex situations (because rmtree is fundamentally more complex than, say, walk)