cpython icon indicating copy to clipboard operation
cpython copied to clipboard

GH-101357: Suppress `OSError` from `pathlib.Path.exists()` and `is_*()`

Open barneygale opened this issue 1 year ago • 4 comments

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.

  • Issue: gh-101357

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

barneygale avatar Apr 24 '24 19:04 barneygale

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

nineteendo avatar Apr 24 '24 19:04 nineteendo

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 avatar Apr 24 '24 20:04 barneygale

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?

zooba avatar May 10 '24 09:05 zooba

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.

barneygale avatar May 10 '24 18:05 barneygale

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.

zooba avatar May 13 '24 11:05 zooba

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

barneygale avatar May 13 '24 12:05 barneygale

Thanks very much for the review!

barneygale avatar May 14 '24 17:05 barneygale