Skip to content

update subprocess module stub for Python 3.5 #426

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

Merged
merged 8 commits into from
Aug 6, 2016

Conversation

jdelic
Copy link
Contributor

@jdelic jdelic commented Aug 1, 2016

I'm marking this as WIP, because it's full of sys.version_info branches and that looks suboptimal to me.

Since many of the methods share the same argument lists... can mypy handle assignments correctly? It seems to me that a single sys.version_info branched declaration of call could be assigned to check_call and check_output to shorten the stub significantly. As in:

if sys.version_info >= (3, 4):
    def call(...) -> ...
else:
    def call(...) -> ...

check_call = call

Would that work?

Otherwise (assuming the tests pass) I think this can be merged in the state it is in now.

@jdelic
Copy link
Contributor Author

jdelic commented Aug 1, 2016

I just realized that check_output has a different return type from call so I don't think that the stub would shorten significantly unless there is a trick that I don't know about for reusing a list of arguments.

@jdelic jdelic changed the title [WIP] update subprocess module stub for Python 3.5 update subprocess module stub for Python 3.5 Aug 5, 2016
@jdelic
Copy link
Contributor Author

jdelic commented Aug 5, 2016

The tests now fail because of python/mypy#1168 (and related issues) 😒, but otherwise this should be good to go.

@gvanrossum
Copy link
Member

gvanrossum commented Aug 5, 2016 via email

@jdelic
Copy link
Contributor Author

jdelic commented Aug 5, 2016

Not so fast with the blame. sys.version_info checks now work, and mypy only analyzes one side. If you're still getting this error you either are using a version check that mypy doesn't recognize, or you have some other kind of error that causes mypy to consider multiple variants. So please look at your code some more.

Is this fixed on mypy master only? Because I get this error all over the place in typeshed master with mypy 0.4.3, so I assumed that it's still unfixed (for example in https://github.com/python/typeshed/blob/master/stdlib/3/email/__init__.pyi#L20-L29). Or is typeshed just "currently buggy"?

@gvanrossum
Copy link
Member

It's fixed in mypy master, but there's not going to be a release for
another month. Are you running typeshed's tests with mypy 0.4.3 from PyPI?
That's not how we're running then in Travis CI, you're supposed to run the
tests with mypy from master.

On Fri, Aug 5, 2016 at 3:13 PM, jdelic [email protected] wrote:

Not so fast with the blame. sys.version_info checks now work, and mypy
only analyzes one side. If you're still getting this error you either are
using a version check that mypy doesn't recognize, or you have some other
kind of error that causes mypy to consider multiple variants. So please
look at your code some more.

Is this fixed on mypy master only? Because I get this error all over the
place in typeshed master, so I assumed that it's still unfixed (for
example in https://github.com/python/typeshed/blob/master/stdlib/3/
email/init.pyi#L20-L29). Or is typeshed just "currently buggy"?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#426 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACwrMjutAbvLAs7hoWfWUGHhVHnk4wA-ks5qc7WfgaJpZM4JaDu2
.

--Guido van Rossum (python.org/~guido)

@jdelic
Copy link
Contributor Author

jdelic commented Aug 5, 2016

It's fixed in mypy master, but there's not going to be a release for another month. Are you running typeshed's tests with mypy 0.4.3 from PyPI? That's not how we're running then in Travis CI, you're supposed to run the tests with mypy from master.

whoops :). I will look at my code some more.

@jdelic
Copy link
Contributor Author

jdelic commented Aug 5, 2016

... aaaand fixed.

@gvanrossum
Copy link
Member

Can you merge again from upstream master? It seems there's a merge conflict due to #438 (also yours).

@jdelic
Copy link
Contributor Author

jdelic commented Aug 5, 2016

Can you merge again from upstream master? It seems there's a merge conflict due to #438 (also yours).

I don't see a merge conflict, though 😕. And #438 wasn't mine. I updated from master, but that only pulled in 5de7bfe.

@gvanrossum
Copy link
Member

OK, sorry for the mixed identity. I'll resolve this myself.

@gvanrossum gvanrossum merged commit 1ac3c2f into python:master Aug 6, 2016
@gvanrossum
Copy link
Member

Thanks! All set. (I've made one more final tiny cleanup, pushed as a separate commit, cc1f921.)

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.

2 participants