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

Accept pathlib.Path objects for stdout/stderr #598

Merged
merged 3 commits into from
Jun 13, 2022

Conversation

MarSoft
Copy link
Contributor

@MarSoft MarSoft commented Feb 5, 2022

Fix #597.
I decided to not add an additional import for pathlib.Path and instead check if the object passed is "path-like" according to PEP-519.

Fix amoffat#597.
I decided to not add an additional import for `pathlib.Path` and instead check if the object passed is "path-like" according to [PEP-519](https://www.python.org/dev/peps/pep-0519/).
@amoffat
Copy link
Owner

amoffat commented Feb 6, 2022

Thanks for documenting the issue and starting a PR. Would you mind adding some tests around the usage of a Path object please?

@mentalisttraceur
Copy link

mentalisttraceur commented May 27, 2022

Code review nit: the Pythonic way to resolve magic methods is to look for them on the class, not on the instance:

-hasattr(out, '__fspath__')
+hasattr(type(out), '__fspath__')

Well, even more Pythonic is to just use isinstance with the abstract base class:

isinstance(out, os.PathLike)

While Python 3.5 introduced pathlib, __fspath__ wasn't introduced until Python 3.6
@mentalisttraceur
Copy link

mentalisttraceur commented May 27, 2022

Based on the comment in #579 about still supporting Python 2, isinstance(out, os.PathLike) is out, so hasattr(type(out), '__fspath__') is probably the closest to the Right(tm) test we can do with the same code on both 2 and 3.

@ecederstrand
Copy link
Collaborator

I've added a test case now.

While Python 3.5 introduced pathlib, __fspath__ wasn't introduced until Python 3.6. Therefore, using Path objects on Python 3.5 still won't work. I think that's an acceptable discrepancy.

@ecederstrand ecederstrand requested a review from amoffat May 27, 2022 21:41
@ecederstrand ecederstrand merged commit 89154ae into amoffat:develop Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accept pathlib.Path objects for _out and _err
4 participants