Skip to content

Conversation

@Quinn-With-Two-Ns
Copy link
Contributor

Add ApplicationFailure.Builder to handle the growing number of options on ApplicationFailure

@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team as a code owner April 30, 2025 21:37
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Nothing blocking

*
* <p>Default is {@link ApplicationErrorCategory#UNSPECIFIED}.
*/
public Builder setCategory(ApplicationErrorCategory category) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should change the getter on ApplicationFailure to getCategory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also be fine changing the setter instead, any preference?

Copy link
Member

Choose a reason for hiding this comment

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

+1 to the shorter version

* @param details optional details about the failure. They are serialized using the same approach
* as arguments and results.
*/
public static ApplicationFailure newFailureWithCategory(
Copy link
Member

Choose a reason for hiding this comment

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

I thought this would be a breaking change but this isn't actually released, for any other readers.

*
* <p>Default is {@link ApplicationErrorCategory#UNSPECIFIED}.
*/
public Builder setCategory(ApplicationErrorCategory category) {
Copy link
Member

Choose a reason for hiding this comment

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

+1 to the shorter version

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit 537d99d into temporalio:master May 1, 2025
9 checks passed
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