fix: don't set excepthook unless pretty_exceptions_enable #1030
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, pretty_exceptions_enable means "set sys.excepthook to the default exception handler". This commit changes the interpretation to "simply don't change sys.excepthook at all". If it was the default handler before, it will remain the default handler. If it was a customized excepthook before, it will remain a customized excepthook. This interpretation gives more control to users when pretty_exceptions_enable is False, while maintaining the same out-of-the-box experience for users who leave it at its default (True). Since sys.excepthook is for uncaught exceptions, it will be read directly by humans, I don't expect this change to break any users of Typer. Most users would leave pretty_exceptions_enable to the default (True), and that behavior did not change at all in this commit.
|
Hi @charmoniumQ, thanks for the contribution! Currently this PR is failing the CI - there appear to be some failing tests. Could you look into those? It would be much more straightforward to review this PR and its proposed changes once all unit tests are green. Thanks! 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, pretty_exceptions_enable means "set sys.excepthook to the default exception handler". This commit changes the interpretation to "simply don't change sys.excepthook at all". If it was the default handler before, it will remain the default handler. If it was a customized excepthook before, it will remain a customized excepthook.
This interpretation gives more control to users when pretty_exceptions_enable is False, while maintaining the same out-of-the-box experience for users who leave it at its default (True).
Since sys.excepthook is for uncaught exceptions, it will be read directly by humans, I don't expect this change to break any users of Typer. Most users would leave pretty_exceptions_enable to the default (True), and that behavior did not change at all in this commit.