Skip to content

Support generic os.PathLike instead of pathlib.Path#11580

Merged
mhvk merged 3 commits into
astropy:mainfrom
olebole:os-pathlike
Apr 16, 2021
Merged

Support generic os.PathLike instead of pathlib.Path#11580
mhvk merged 3 commits into
astropy:mainfrom
olebole:os-pathlike

Conversation

@olebole

@olebole olebole commented Apr 16, 2021

Copy link
Copy Markdown
Member

Description

This PR replaces all appearances of

 if isinstance(fileobj, pathlib.Path): 
     fileobj = str(fileobj)

with

if isinstance(fileobs, os.PathLike):
    fileobj = fileobj.__fspath__()

Rationale: In Python, paths are not necessarily strings or pathlib.Path objects, but anything that is os.Pathlike is acceptable in open(). One use case here is a specific path like object that retrieves/builds a file just before it is opened (f.e. for a cache).
The canonical way to access the file path on the OS is then done by __fspath__(), not by str().

Fixes #11579

@Cadair

Cadair commented Apr 16, 2021

Copy link
Copy Markdown
Member

direct use of __fspath__ feels odd. What's the difference between that and str()?

@olebole

olebole commented Apr 16, 2021

Copy link
Copy Markdown
Member Author

@Cadair There is no guarantee that str() actually returns the path in the file system. This is true for pathlib.Path, but other os.PathLike classes may not even implement a proper __str__() method to return the path. For example, the DummyPath class in #11579 is a valid implementation of os.PathLike, but would fail when using str().

In contrast, __fspath__() is a well-defined interface, so I don't see why one shouldn't use it.

@Cadair

Cadair commented Apr 16, 2021

Copy link
Copy Markdown
Member

That makes sense, but seems odd that class doesn't have a non-magic method for doing that.

@adrn

adrn commented Apr 16, 2021

Copy link
Copy Markdown
Member

Nice! I think this is a great idea, and is almost a bug fix. Could you add a test, even taken from your original issue?

@pllim pllim added this to the v4.3 milestone Apr 16, 2021
pllim
pllim reviewed Apr 16, 2021
View reviewed changes
Comment thread docs/changes/io.fits/11580.feature.rst Outdated
@pllim

pllim commented Apr 16, 2021

Copy link
Copy Markdown
Member

Thanks! Agree with @adrn that new tests are needed to cover the new changes.

@pllim

pllim commented Apr 16, 2021

Copy link
Copy Markdown
Member

Also perhaps add an example in user-facing doc? Otherwise, this new improvement is hard to discover.

@saimn

saimn commented Apr 16, 2021

Copy link
Copy Markdown
Contributor

In contrast, fspath() is a well-defined interface, so I don't see why one shouldn't use it.

Indeed, but then you should use os.fspath instead:
https://docs.python.org/3/whatsnew/3.6.html#pep-519-adding-a-file-system-path-protocol
https://docs.python.org/3/glossary.html#term-path-like-object

@olebole

olebole commented Apr 16, 2021

Copy link
Copy Markdown
Member Author

Thank you for the suggestions! I now fixed the changelog RST, and replaced __fspath__() with os.fspath(). I will add a simple test later.
Concerning an example in user-faced doc: I don't have a good, simple example that could serve here. My own use case is a Gitlab file cache (to overcome the problem that one cannot easily selectively mount some files from a Git repository via submodule), but I have no 10-lines example. Suggestions would be welcome here.

@pep8speaks

pep8speaks commented Apr 16, 2021 •
edited
Loading

Copy link
Copy Markdown

Hello @olebole 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-04-16 16:18:40 UTC

@olebole olebole closed this Apr 16, 2021
@olebole olebole reopened this Apr 16, 2021
@olebole olebole changed the title WIP: Support generic os.PathLike instead of pathlib.Path Support generic os.PathLike instead of pathlib.Path Apr 16, 2021
mhvk
mhvk approved these changes Apr 16, 2021
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe best for @Cadair to do a proper review, but this looks all OK to me. Nice!

saimn
saimn approved these changes Apr 16, 2021
View reviewed changes

@saimn saimn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, thanks @olebole !

@mhvk

mhvk commented Apr 16, 2021

Copy link
Copy Markdown
Contributor

Two approvals would seem good enough. Let's get it in. Thanks, @olebole!

@mhvk mhvk merged commit d010ef3 into astropy:main Apr 16, 2021
@nstarman

nstarman commented Apr 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Oh no! Stuff in the docs should now be "path-like", see #11118.

nstarman
nstarman reviewed Apr 16, 2021
View reviewed changes
Comment thread astropy/io/fits/util.py

from os import PathLike
path_like = (str, PathLike)
path_like = (str, os.PathLike)

@nstarman nstarman Apr 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

according to https://docs.python.org/3/glossary.html#term-path-like-object, path-like should include bytes

@nstarman nstarman Apr 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The three components of python's path-like are string, byte, os.PathLike

Comment thread astropy/io/registry.py
_identifiers = OrderedDict()

PATH_TYPES = (str, pathlib.Path)
PATH_TYPES = (str, os.PathLike)

@nstarman nstarman Apr 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same. should include bytes

@nstarman nstarman mentioned this pull request Apr 16, 2021
Open
2 tasks
@nstarman

nstarman commented Apr 16, 2021

Copy link
Copy Markdown
Member

I've added this as a TODO in the above issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@nstarman nstarman nstarman left review comments
@saimn saimn saimn approved these changes
@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

config io.fits io.registry utils

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

Please support os.PathLike

8 participants

@olebole @Cadair @adrn @pllim @saimn @pep8speaks @mhvk @nstarman