-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
add __fspath__ support to os.path #2053
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
Conversation
Fixes python#1997. This is tricky because we need to get the return values right (see python#1960 for prior attempts) and we often run into python/mypy#3644. I found that I could express most signatures correctly using a series of overloads. A few other changes in here: - Added splitunc, which according to https://docs.python.org/3/library/os.path.html should exist in both Unix and Windows. - Made the second argument to os.path.curdir Optional to match the implementation. - Fixed os.path.split, whose previous Path-aware signature triggered python/mypy#3644. I used the following test program to make sure mypy accepted the signatures used here:
@overload | ||
def relpath(path: _StrPath, start: Optional[_StrPath] = ...) -> Text: ... # type: ignore | ||
@overload | ||
def relpath(path: _BytesPath, start: _BytesPath) -> bytes: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relpath
isn't guarded by a 3.6 version check and there is no AnyStr fallback.
Also, the bytes variant is missing Optional
.
(Applies to the 2 version as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is intentional. _StrPath
is just Text
below 3.6, so this implementation is correct there.
If start is None, the code uses os.curdir, which is Text, so the path also has to be Text. Although come to think of it, we may need something more complicated in Python 2, where you can mix bytes and Text paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the current code already works in Python 2 (modulo a mypy bug where some overloads silently fall back to Any).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, merge whenever you want to.
Thanks for the review! |
Fixes python#1997, python#2068. This is tricky because we need to get the return values right (see python#1960 for prior attempts) and we often run into python/mypy#3644. I found that I could express most signatures correctly using a series of overloads. A few other changes in here: - Added splitunc, which according to https://docs.python.org/3/library/os.path.html should exist in both Unix and Windows. - Made the second argument to os.path.curdir Optional to match the implementation. - Fixed os.path.split, whose previous Path-aware signature triggered python/mypy#3644.
Fixes python#1997, python#2068. This is tricky because we need to get the return values right (see python#1960 for prior attempts) and we often run into python/mypy#3644. I found that I could express most signatures correctly using a series of overloads. A few other changes in here: - Added splitunc, which according to https://docs.python.org/3/library/os.path.html should exist in both Unix and Windows. - Made the second argument to os.path.curdir Optional to match the implementation. - Fixed os.path.split, whose previous Path-aware signature triggered python/mypy#3644.
Fixes #1997, #2068.
This is tricky because we need to get the return values right (see #1960 for
prior attempts) and we often run into python/mypy#3644. I found that I
could express most signatures correctly using a series of overloads.
A few other changes in here:
should exist in both Unix and Windows.
I couldn't come up with a correct signature for
os.path.commonpath
, so I am falling back to using Any as its return type.I used the following test program to make sure mypy accepted the signatures used here:
I made the same changes to the Python 2 stub in order to keep the two consistent.