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

gh-119993 ignore NotADirectoryError in Path.unlink() if missing_ok is True #120049

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MusicalNinjaDad
Copy link

@MusicalNinjaDad MusicalNinjaDad commented Jun 4, 2024

Add a short note on when Path.unlink() raises NotADirectoryError

This can occur "in the wild", for example, after calling shutil.move(src, dst) where dst is a non-existent directory. Debugging can cost a lot of time as the cause can be non-intuitive, potentially not nearby in the code and the docs don't mention the exception as being raised in any circumstances.
(This just happened to me after encountering pypa/cibuildwheel#1850)

Fixes: #119993


📚 Documentation preview 📚: https://cpython-previews--120049.org.readthedocs.build/

Doc/library/pathlib.rst Outdated Show resolved Hide resolved
@MusicalNinjaDad MusicalNinjaDad force-pushed the MusicalNinjaDad/issue119993 branch from 613cdb2 to ce3577d Compare July 4, 2024 12:04
@MusicalNinjaDad MusicalNinjaDad changed the title gh-119993 Document when NotADirectoryError is raised by Path.unlink() gh-119993 ignore NotADirectoryError in Path.unlink() if missing_ok is `True Jul 4, 2024
@MusicalNinjaDad MusicalNinjaDad changed the title gh-119993 ignore NotADirectoryError in Path.unlink() if missing_ok is `True gh-119993 ignore NotADirectoryError in Path.unlink() if missing_ok is True Jul 4, 2024
@MusicalNinjaDad
Copy link
Author

MusicalNinjaDad commented Jul 4, 2024

@barneygale - this is how unlink would/could work if it were to ignore NotADirectoryError. Semantically that makes success with missing_ok = True mean "The file is not there afterwards" and with missing_ok = False to mean "The file was deleted". I think this makes sense, but I have no strong feelings that it must be like this.

Windows works differently from Linux/MacOS and raises FileNotFoundError if an intermediate path refers to a file. Without this change the same end-user code will potentially raise on one OS and not on another for the same situation.

Doc/library/pathlib.rst Show resolved Hide resolved
Doc/library/pathlib.rst Show resolved Hide resolved
Doc/library/pathlib.rst Show resolved Hide resolved
Lib/test/test_pathlib/test_pathlib.py Show resolved Hide resolved
Comment on lines +796 to +797
if sys.platform.startswith("win"):
self.assertFileNotFound(p.unlink)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a @needs_posix decorator to the test and don't bother testing Windows IMO :]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this hurt to have it in? Isn't it generally better to have the extra test coverage, or are we really struggling to keep execution times down?

@bedevere-app
Copy link

bedevere-app bot commented Aug 10, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
2 participants