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-101357: Suppress OSError from pathlib.Path.exists() and is_*() #118243

Merged
merged 7 commits into from
May 14, 2024

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Apr 24, 2024

Suppress all OSError exceptions from pathlib.Path.exists() and is_*() rather than a selection of more common errors as we do presently. Also adjust the implementations to call os.path.exists() etc, which are much faster on Windows thanks to GH-101196.


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

…`is_*()`

Suppress all `OSError` exceptions from `pathlib.Path.exists()` and `is_*()`
rather than a selection of more common errors as we do presently. Also
adjust the implementations to call `os.path.exists()` etc, which are much
faster on Windows thanks to pythonGH-101196.
@nineteendo
Copy link
Contributor

What do you intend to do with #117858? This pull request includes all its changes (except for the news entry).

@barneygale
Copy link
Contributor Author

They have some overlap but they're not quite the same:

  • The other PR depends on your lexist() speedup landing, whereas this PR does not
  • The other PR maintains pathlib's existing interface, whereas this PR changes something that might be important, and so this PR is more likely to be rejected by reviewers or otherwise spend a long time in review.

@barneygale barneygale marked this pull request as ready for review May 8, 2024 18:51
@barneygale barneygale requested a review from zooba May 8, 2024 18:53
@zooba
Copy link
Member

zooba commented May 10, 2024

Is it worth pointing out that this brings them into line with the os.path equivalents? And maybe suggesting the right API (I assume stat/lstat) to get more detailed error.

There might also be value in some kind of conceptual comment that these functions mean what they say if they return True, but could mean multiple things if they return False, and hence if you're using them you should be testing for True?

@barneygale
Copy link
Contributor Author

barneygale commented May 10, 2024

Thanks for the feedback!

There might also be value in some kind of conceptual comment that these functions mean what they say if they return True, but could mean multiple things if they return False, and hence if you're using them you should be testing for True?

It seems quite redundant to me, as the existing docs for each method are clear that False is returned if the path is inaccessible for any reason.

edit: re-reading this comment, I think I came across too strong. If you think it's important, I can include some conceptual remarks as you suggest.

@zooba
Copy link
Member

zooba commented May 13, 2024

If you think it's important, I can include some conceptual remarks as you suggest.

I think it's helpful for people who come to our docs not just for reference, but to find out what they should do. But I also can't come up with great wording that doesn't have so many qualifiers that you'd need to be an expert to understand it anyway.

Maybe it's a matter of phrasing it more like: "Returns True if the path refers to a file; otherwise, False. Paths that are inaccessible will return False, as will those that are not files. Use exists to determine whether the path is accessible, and is_dir or is_link to check for directories or symlinks."

Basically, "if you use this, here's what you'll get; if you actually wanted this, here's what you should do". But it's more of a rewrite, and maybe not worth your time or interest. I'm not going to force it, the current PR is fine.

@barneygale
Copy link
Contributor Author

Makes sense! I'll experiment and see if I can make that sort of thing work. Ta!

Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

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

Love it. But do consider other feedback as well, if there's something that can make it even more approachable.

@barneygale
Copy link
Contributor Author

Thanks very much for the review!

@barneygale barneygale enabled auto-merge (squash) May 14, 2024 17:37
@barneygale barneygale merged commit fbe6a09 into python:main May 14, 2024
33 checks passed
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…`is_*()` (python#118243)

Suppress all `OSError` exceptions from `pathlib.Path.exists()` and `is_*()`
rather than a selection of more common errors as we do presently. Also
adjust the implementations to call `os.path.exists()` etc, which are much
faster on Windows thanks to pythonGH-101196.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance or resource usage topic-pathlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants