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

Merge stdlib/{2,3}/os/{__init__,path}.pyi #1111

Closed
wants to merge 5 commits into from

Conversation

euresti
Copy link
Contributor

@euresti euresti commented Mar 29, 2017

This is the full merge of os and os.path in one PR. With this the files are identical and could easily be renamed into stdlib/2and3

@gvanrossum
Copy link
Member

So this supersedes #1103 right? Does it contain the changes requested in the review there? Can you close #1103?

@gvanrossum
Copy link
Member

So there's still a problem here -- are you aware that there's also a definition of stat_result in posix.pyi? There are quite a few duplicates there -- it doesn't matter for the constants (e.g. O_APPEND) but it does match for the classes, since I've found code (in our own codebase) using posix.stats_result in type annotations. I expect it may be ditto for posixpath.pyi.

@gvanrossum
Copy link
Member

Anther issue: I found a few expressions like this:

os.stat(path)[stat.ST_MODE]  # E: Tuple index must be an integer literal

Now arguably this should be replaced with

os.stat(path).st_mode

But the old code should still be considered valid IMO.

I'm not sure how to fix this other than making mypy somehow recognize constant definitions like ST_MODE = 0 in stats.pyi. (BTW the constants there seem wrong -- to work with the NamedTuple they should be the correct indices, if we fix mypy to support that.)

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

See discussion in main thread of PR.

@JelleZijlstra
Copy link
Member

This merge conflicts now because I merged #1103.

Currently the stubs for posix.pyi in Python 2 are fairly complete, but the ones in Python 3 just re-export stat_result from os.pyi. In reality the os module mostly re-exports names from posix (or from nt on windows?), so maybe ideally the os stub would just have a big from posix import x as x, y as y.

@euresti
Copy link
Contributor Author

euresti commented Mar 31, 2017

I tried bringing everything into posix.pyi but hit a small hitch if I wanted to just do from posix import *, which is what os.py does in cpython, basically posix.environ is a Dict[bytes, bytes] but os.environ is a different beast and mypy gives me an import error. Technically, like 90% of the methods and constants in os are imported from posix so I'd rather not do from posix import O_CREATE, S_OK, etc any tips on handling this?

@JelleZijlstra
Copy link
Member

I see, that's unfortunate. I can't really think of a better solution than repeating all the names.

@euresti
Copy link
Contributor Author

euresti commented Apr 3, 2017

I'm gonna close this PR and split it up into smaller parts.

@euresti euresti closed this Apr 3, 2017
@euresti euresti deleted the os_path_merge branch May 4, 2018 14:12
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.

3 participants