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

Fix stdlib subprocess.Popen.returncode typing #7806

Merged
merged 3 commits into from
May 8, 2022
Merged

Fix stdlib subprocess.Popen.returncode typing #7806

merged 3 commits into from
May 8, 2022

Conversation

mnixry
Copy link
Contributor

@mnixry mnixry commented May 8, 2022

According to the document, the Popen.returncode should be Optional[int]:

The child return code, set by poll() and wait() (and indirectly by communicate()). A None value indicates that the process hasn’t terminated yet.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented May 8, 2022

The problem with this change is that there are situations where the caller knows that the exit code can't be None, see the mypy primer output above. That said, we should at least change the return type to int | Any to cover the None case. Also obligatory link to python/typing#566.

@mnixry
Copy link
Contributor Author

mnixry commented May 8, 2022

The problem with this change is that there are situations where the caller knows that the exit code can't be None, see the mypy primer output above.

Thank you for your explanation.

So according to my understanding, using int instead of Optional<int> is a compromised solution to keep compatibility with the existing code?
I just remember that the type was int | None some time ago, this makes the recent change reasonable for me.

@srittau
Copy link
Collaborator

srittau commented May 8, 2022

So according to my understanding, using int instead of Optional<int> is a compromised solution to keep compatibility with the existing code?

A mixture of compatibility with existing code, but also not requiring authors of new code to have to add assert in places like this, even though a human reader can clearly spot that the value can't be None.

Edit: But int is also clearly wrong, which is why int | Any would be the best we could do at the moment.

@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hauntsaninja hauntsaninja merged commit 32f474d into python:master May 8, 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.

4 participants