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

Error handling refactor #1280

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Error handling refactor #1280

merged 1 commit into from
Nov 18, 2024

Conversation

neekolas
Copy link
Contributor

@neekolas neekolas commented Nov 18, 2024

tl;dr

  • Refactors MessageProcessingError into GroupMessageProcessingError that is more narrowly scoped
  • Uses GroupError to handle errors when processing Welcomes
  • Make process_for_id take a generic error type to support both
  • Trim down unused error variants
  • Make RetryableError explicit and don't use _ => false

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@neekolas neekolas changed the title Error handling refactor [WIP] Error handling refactor Nov 18, 2024
@neekolas neekolas force-pushed the 11-17-error_handling_refactor branch 5 times, most recently from cbce3c5 to 0cb760e Compare November 18, 2024 21:10
@neekolas neekolas changed the title [WIP] Error handling refactor Error handling refactor Nov 18, 2024
@neekolas neekolas force-pushed the 11-17-error_handling_refactor branch from 0cb760e to b12ac5c Compare November 18, 2024 21:19
@neekolas neekolas marked this pull request as ready for review November 18, 2024 21:19
@neekolas neekolas requested a review from a team as a code owner November 18, 2024 21:19
@neekolas neekolas force-pushed the 11-17-error_handling_refactor branch from b12ac5c to 63e993b Compare November 18, 2024 21:23
EncodeProto(#[from] prost::EncodeError),
#[error("proto decode error: {0}")]
DecodeProto(#[from] prost::DecodeError),
#[error(transparent)]
Copy link
Contributor

@mchenani mchenani Nov 18, 2024

Choose a reason for hiding this comment

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

question: in the previous code we have this comment:
#[error("intent error: {0}")] Intent(#[from] IntentError),
is this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably be doing more transparent error wrapping, since the additional comments usually add little incremental value. But this one was an accident, and that refactor could be handled separately.

@neekolas neekolas force-pushed the 11-17-error_handling_refactor branch from 63e993b to 3a3d31b Compare November 18, 2024 22:11
Copy link
Contributor

@mchenani mchenani left a comment

Choose a reason for hiding this comment

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

Nice cleanup🧹🙌

@neekolas neekolas force-pushed the 11-17-error_handling_refactor branch from 3a3d31b to 70cf5c5 Compare November 18, 2024 22:27
@neekolas neekolas merged commit f049bb5 into main Nov 18, 2024
11 checks passed
@neekolas neekolas deleted the 11-17-error_handling_refactor branch November 18, 2024 23:44
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