Skip to content

fix: Update sys.exit and SystemExit exception to have the same types #8554

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 4 commits into from
Aug 20, 2022
Merged

fix: Update sys.exit and SystemExit exception to have the same types #8554

merged 4 commits into from
Aug 20, 2022

Conversation

kkirsche
Copy link
Contributor

fix: #8513

This updates the types of sys.exit and SystemExit with the recommendations of @Akuli. The issue had received 👍 s from three team members, so I wanted to help move this along for the team.

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. The mypy_primer hits are a little unfortunate, but they're arguably "true positives", and I can live with them.

Two things that might be good to change, before merging:

  1. Presumably builtins.quit() and builtins.exit() should be changed in the same way as sys.exit()?
  2. Can we define a type alias in the sys module for str | int | None, and then use that for all of these functions, and the code attribute of SystemExit? I think it would help ensure that they don't drift out of sync in the future.

@kkirsche
Copy link
Contributor Author

Added 🙂

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll wait a little while before merging to see if any other maintainers have any thoughts :)

@github-actions

This comment has been minimized.

@kkirsche
Copy link
Contributor Author

works for me, thanks for the review

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

cwltool (https://github.com/common-workflow-language/cwltool)
+ cwltool/main.py:1322:24: error: Incompatible return value type (got "Union[str, int, None]", expected "int")  [return-value]
+ tests/util.py: note: In function "get_main_output":
+ tests/util.py:110:14: error: Incompatible types in assignment (expression has type "Union[str, int, None]", variable has type "int")  [assignment]

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/cmd/quickstart.py: note: In function "main":
+ sphinx/cmd/quickstart.py:551:16: error: Incompatible return value type (got "Union[str, int]", expected "int")  [return-value]

mypy (https://github.com/python/mypy)
+ mypy/api.py:62: error: Incompatible types in assignment (expression has type "Union[str, int, None]", variable has type "int")  [assignment]

@AlexWaygood AlexWaygood merged commit b26c31a into python:master Aug 20, 2022
@kkirsche kkirsche deleted the fix/8513 branch August 20, 2022 10:04
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.

sys.exit and SystemExit
2 participants