ENH: Add support for third-party path-like objects by backporting os.fspath #12157
Conversation
| fid = file.open("rb") | ||
| own_fid = True | ||
| else: | ||
| if hasattr(file, 'read'): | ||
| fid = file | ||
| own_fid = False |
There was a problem hiding this comment.
Some alternatives to this block, and the similar ones below
This patch:
if hasattr(file, 'read'):
fid = file
own_fid = False
else:
fid = open(os_fspath(file), "rb")
own_fid = TrueAlternative 1 (smaller diff);
if isinstance(file, os_PathLike):
file = os_fspath(file)
if isinstance(file, basestring):
fid = open(file, "rb")
own_fid = True
else:
fid = file
own_fid = FalseAlternative 2 (no duck-typing):
if isinstance(file, io.IOBase):
fid = file
own_fid = False
else:
fid = open(os_fspath(file), "rb")
own_fid = True
| elif is_pathlib_path(filename): | ||
| if is_pathlib_path(filename): | ||
| # special case - if we were constructed with a pathlib.path, | ||
| # then filename is a path object, not a string | ||
| self.filename = filename.resolve() |
There was a problem hiding this comment.
This is the only remaining use of is_pathlib_path, which is needed for a weird special case.
There was a problem hiding this comment.
Since we need is_pathlib_path anyway, PR #11348 more straight-forward and simpler.
There was a problem hiding this comment.
This is orthogonal to PR #11348 - this makes numpy work with backports of pathlib too / third-party objects implementing __fspath__ too.
eric-wieser
force-pushed
the
fspath
branch
2 times, most recently
from
34a4160 to
a47aa9e
Compare
|
Azure is missing from the CI list on this and some other recent PRs -- I'll open an issue. |
|
Coverage failures are garbage, and caused by us not collecting coverage data in python < 3.6 |
|
@tylerjereddy: did you find a workaround to make the CI run? |
|
Closing/reopening to restart CI |
|
@eric-wieser No, but maybe Microsoft did something since their engineers were investigating. Looks like Azure is running on all recent PR triggers including updates / force pushes, etc. |
|
Note that the only way to make CI pass here is to collect and merge coverage data on multiple python versions - it's impossible to exercise python3.5-only code if we only collect coverage data on 3.6. Perhaps we should exclude |
It would be nice to cover the other Python versions, but not sure if it is worth the effort and / or risk that we break the fragile codecov stuff as happened with SciPy. I'd probably lean toward leaving most files in the report but having the reviewer skeptical of cov-related failures, but probably not a big issue either way as long it is not a big slippery slope with progressive blacklisting of lots of files on the report. |
|
At this point I prefer the current situation - when codecov fails the commiter/reviewer asks to ignore the fail. |
|
LGTM - will merge soon if there are no other comments |
|
Thanks Eric |
In response to #11348