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

Update open, os.fspath, os.fsencode, os.fsdecode, pathlib.PurePath and pathlib.Path stubs due to PEP-519 #991

Merged
merged 5 commits into from
Mar 21, 2017

Conversation

sproshev
Copy link
Contributor

Update stubs for:

  • builtin open function
  • os.path, os.fsencode and os.fsdecode functions
  • pathlib.PurePath and pathlib.Path classes

due to PEP-519.

@JelleZijlstra
Copy link
Member

This change looks good to me but there's a lot more to be done in relation to PEP 519.

First, many additional functions should be updated to accept os.PathLike in addition to str for paths. We should maybe add a type alias (typing.Path?) for Union[str, os.PathLike] to typing, because otherwise we'll have to put a lot of version checks for 3.6 in various places.

Also, os.PathLike should perhaps be generic over AnyStr to express the bytes/str situation more precisely. For example, the Path constructor only accepts os.PathLike[str], not os.PathLike[bytes]. On the other hand, most use cases for Paths probably don't care about bytes paths, so maybe we don't need this complication.

Anyway, those issues are outside the scope of this PR. I'll think about them more and maybe write up a proposal.

def fsencode(filename: str) -> bytes: ...
def fsdecode(filename: bytes) -> str: ...
if sys.version_info >= (3, 6):
def fsencode(filename: Union[str, PathLike]) -> bytes: ...
Copy link
Member

Choose a reason for hiding this comment

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

It actually also accepts bytes, and this is documented at https://docs.python.org/3/library/os.html#os.fsencode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

**kwargs: Any) -> _P: ...
if sys.version_info < (3, 6):
def __new__(cls: Type[_P], *args: Union[str, PurePath],
**kwargs: Any) -> _P: ...
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, apparently Path takes arbitrary kwargs and then ignores them. This doesn't appear to be documented. Do you know why this is? I might open a bug against CPython about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I think it is better to open a bug.

Copy link
Member

Choose a reason for hiding this comment

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

yan12125 pushed a commit to yan12125/python3-android that referenced this pull request Mar 18, 2017
@sproshev sproshev changed the title PEP-519 Update open, os.fspath, os.fsencode, os.fsdecode, pathlib.PurePath and pathlib.Path stubs due to PEP-519 Mar 18, 2017
@overload
def fspath(path: bytes) -> bytes: ...
@overload
def fspath(path: PathLike) -> Union[str, bytes]: ...
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to have this overload return Any since Union return types are troublesome. Sorry for not noticing this before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@JelleZijlstra JelleZijlstra merged commit 25464cf into python:master Mar 21, 2017
@JelleZijlstra
Copy link
Member

Thanks!

@sproshev sproshev deleted the sproshev/pep-519 branch March 21, 2017 15:58
@gvanrossum
Copy link
Member

Sadly I have to revert one part of this PR -- the changes to builtins.pyi introduced a new element in mypy's "bootstrap" import cycle (by adding a from os import PathLike to it). I will rewrite it as follows:

  • define _PathLike in builtins
  • use that in the definition of open()
  • import that as PathLike into builtins

All that only for Python 3.6.

gvanrossum pushed a commit that referenced this pull request Mar 21, 2017
This reverts commit 48b271c.

See #991 (comment)
for details; basically this disturbed mypy's bootstrap import cycle.
@JelleZijlstra
Copy link
Member

Oh no. :( Why didn't Travis catch this?

@gvanrossum
Copy link
Member

Good question. I think the travis mypy test script runs mypy with a whole bunch of filenames (of stub files) as arguments, and that may force a different processing order. Maybe we should add a plain mypy -c pass run?

@ambv
Copy link
Contributor

ambv commented Mar 21, 2017

Yeah, it would be totally worth it to run mypy's own testsuite against typeshed. There are two risks:

  • is mypy master stable enough for this exercise? I think it's decently stable, most of issues would come from chicken-and-egg problems when support needs to be added on both ends.
  • are mypy tests fast enough? I think they're decently fast, we've done some work on them.

This would make the life of Dropboxers easier by providing early warnings if something would fail when syncing the submodule.

@gvanrossum
Copy link
Member

Can either of you create a new issue for this? I don't like new discussions under closed issues/PRs if it's not directly "this broke X".

facebook-github-bot pushed a commit to facebook/pyre-check that referenced this pull request Jun 11, 2018
Summary: python/typeshed#991 (comment)

Reviewed By: sinancepel

Differential Revision: D8335824

fbshipit-source-id: 7926319534b9e0bb8039d7786a1dc56a9da2eeb5
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.

None yet

4 participants