Skip to content

Minor exception-related updates #75

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 3 commits into from
Jul 14, 2022
Merged

Conversation

cretz
Copy link
Member

@cretz cretz commented Jul 14, 2022

What was changed

  • Alter ApplicationError to prepend the type to the message if present when showing the string format
  • Change workflow activation failures from ERROR to WARN
  • Change activity failures from DEBUG to WARN
  • Add example in test case showing how to append stack trace

Checklist

  1. Relates to Deserialize traceback from stack trace string in Temporal failures if able #58

@@ -65,10 +68,14 @@ def __init__(
message: str,
*details: Any,
type: Optional[str] = None,
non_retryable: bool = False
non_retryable: bool = False,
Copy link
Member

Choose a reason for hiding this comment

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

Since we control the default value can we change this to retryable: bool = True to improve readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like the init param names to match the property names. Currently we expose Java isNonRetryable, Go NonRetryable, TS nonRetryable, etc.

super().__init__(
message,
# If there is a type, prepend it to the message on the string repr
exc_args=(message if not type else f"{type}: {message}",),
Copy link
Member

Choose a reason for hiding this comment

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

Why not override __str__?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to alter the Exception.args not just the string. That way our exception, like basically all other Python exceptions, still has str(exception) == str(exception.args). Doesn't seem to have benefit only alter the string representation.

Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

Overall LGTM

@cretz cretz merged commit 5926b76 into temporalio:main Jul 14, 2022
@cretz cretz deleted the exception-format branch July 14, 2022 21: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.

2 participants