Skip to content
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

pathlib.Path.unlink raises a NotADirectoryError if some element of the path is actually a file - this is undocumented #119993

Open
MusicalNinjaDad opened this issue Jun 3, 2024 · 9 comments · May be fixed by #120049
Labels
docs Documentation in the Doc dir topic-pathlib

Comments

@MusicalNinjaDad
Copy link

MusicalNinjaDad commented Jun 3, 2024

Documentation

https://docs.python.org/3/library/pathlib.html#pathlib.Path.unlink states that a FileNotFoundError will be raised if the path does not exist.
On macos-14 a NotADirectoryError is raised if the directory does not exist.

See pypa/cibuildwheel#1850 for an example.

Linked PRs

@MusicalNinjaDad MusicalNinjaDad added the docs Documentation in the Doc dir label Jun 3, 2024
@henryiii
Copy link
Contributor

henryiii commented Jun 3, 2024

I can’t quickly replicate on macOS 14 Intel or macOS 14 ARM locally. There must be something specific about the setup when it happened in cibuildwheel that I’m not capturing.

@serhiy-storchaka
Copy link
Member

You can get a NotADirectoryError if an intermediate path refers to a file.

>>> os.unlink('/etc/hosts/file')
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    os.unlink('/etc/hosts/file')
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^
NotADirectoryError: [Errno 20] Not a directory: '/etc/hosts/file'

@MusicalNinjaDad
Copy link
Author

Thanks ... I just found that through my debugging too...

Would it be useful to add a comment to that effect in the docs?

@MusicalNinjaDad MusicalNinjaDad changed the title pathlib.Path.unlink raises a NotADirectoryError on macos if the directory does not exist, documentation expects `FileNotFoundError pathlib.Path.unlink raises a NotADirectoryError if some element of the path is actually a file - this is undocumented Jun 4, 2024
@barneygale
Copy link
Contributor

barneygale commented Jun 12, 2024

The error comes from the operating system, and it can occur from many pathlib.Path methods. Other exceptions like PermissionError are also possible. I don't want to start documenting this stuff inline in the pathlib docs because it's hard to know where to draw the line.

This bit is a tiny bit misleading:

If missing_ok is false (the default), FileNotFoundError is raised if the path does not exist.

It could instead say:

If missing_ok is false (the default), FileNotFoundError is raised if the path does not exist, as reported by the operating system.

@MusicalNinjaDad
Copy link
Author

MusicalNinjaDad commented Jun 13, 2024

That makes sense. I can see how it would quickly become unwieldly to inline all the potential exceptions.

How about adjusting the wording to:

This method propagates any :exc:`OSError` encountered during removal.

If *missing_ok* is true, :exc:`FileNotFoundError` exceptions will be
ignored (same behavior as the POSIX ``rm -f`` command).

This way the reader does not expect to only see a FileNotFoundError but is attuned to the possibility of a range of errors being raised.

I've pushed this proposal to the PR in case you think it's OK

@MusicalNinjaDad
Copy link
Author

On a more general note, as a user of pathlib; the real value in the Concrete Paths is the abstraction they provide over os specifics. This abstraction breaks relatively quickly once errors are encountered, particularly in cases where different OS handle scenarios differently.

Would you be interested in adding a short section to the documentation which focusses on this topic as additional help for users? I would be happy to produce a proposal.

@barneygale
Copy link
Contributor

Thinking about it some more... maybe we should treat NotADirectoryError like FileNotFoundError in the code, i.e. suppress the exception when missing_ok=True. The Python test suite's os_helper.unlink() ignores specifically these two exceptions, and I think that NotADirectoryError can only mean that the path doesn't exist. Sorry for flip-flopping on this.

@MusicalNinjaDad
Copy link
Author

MusicalNinjaDad commented Jul 3, 2024

@barneygale, don't worry about "flip-flopping". I often find it's the simple things that are actually hard because of how they slowly highlight subtle things as you work through them.

That's a pretty good idea to suppress NotADirectoryError as logically, this means that the file must be missing - because an intermediate path refers to a file.
I can imagine that there are 1 or 2 additional such cases which semantically are the equivalent of "file missing" ... possibly something related to symlinks ...(Nope, can't see anything when looking through the subclasses of OSError)

I'll let that thought rotate subconsciously for a bit and look at the possibility of suppressing NotADirectoryError - if you like the change then you can remove the docs label, if not, then I can revert the commit (?)

@MusicalNinjaDad
Copy link
Author

Given what I just found in testing, I would suggest that treating NotADirectoryError and FileNotFoundError as equivalent is the right thing to do:

Windows doesn't raise NotADirectoryError. That means that the same code will raise on Mac/Linux but not on Windows if someone uses missing_ok=True and hits the case where an intermediate path refers to a file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir topic-pathlib
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants