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(dispatcher): explicitly disable exception chaining #240

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

lengau
Copy link
Contributor

@lengau lengau commented Feb 15, 2024

In these cases, the chaining doesn't provide useful additional data.

Drive-by: updates ruff to 0.2

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

@lengau lengau requested review from tigarmo and mr-cal February 15, 2024 18:10
Copy link
Contributor

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

LGTM (I didn't test but I am assuming the traceback will look the same)

@lengau
Copy link
Contributor Author

lengau commented Feb 16, 2024

@mr-cal This does change the traceback, since it sets the __context__ to None rather than the previous exception. However, the context exceptions aren't useful to either an end user or a developer using this library since they're implementation details (they're mostly KeyError in places where we use dictionaries and in one case a StopIteration where we call next()). In all cases, these are raising ArgumentParsingError, which the application should always catch and display (but should never care about context).

@lengau lengau requested a review from mr-cal February 16, 2024 15:15
Copy link
Contributor

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

@mr-cal This does change the traceback, since it sets the __context__ to None rather than the previous exception. However, the context exceptions aren't useful to either an end user or a developer using this library since they're implementation details (they're mostly KeyError in places where we use dictionaries and in one case a StopIteration where we call next()). In all cases, these are raising ArgumentParsingError, which the application should always catch and display (but should never care about context).

That sounds reasonable to me, thanks for the explanation!

@tigarmo tigarmo merged commit 4a3ba4d into main Feb 20, 2024
9 checks passed
@tigarmo tigarmo deleted the style/update-ruff branch February 20, 2024 13:14
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