-
Notifications
You must be signed in to change notification settings - Fork 18
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
Flat error handling #757
Flat error handling #757
Conversation
@ChaoticTempest I've fixed the tests |
} | ||
|
||
#[derive(Debug, PartialEq, Eq, Clone, thiserror::Error)] | ||
pub enum Common { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have this pattern, we shouldn't be making a common kind. Instead we should separate out all of these into their own category such as:
enum ProtocolError {
StateNotRunning,
...
}
enum InvalidState {
RequestNotFound,
}
enum InvalidParameters {
InsufficientDeposit,
InsufficientGas,
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChaoticTempest This is opinionated and generates unnecessary code. Error representation for the user will not change, but for us, it can add some confusion. For example, RequestNotFound
is InvalidState
and InvalidParameters
error at the same time. Same about EpochMismatch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I think Common
is too ambiguous of name since that doesn't signify how that error could have happened. I rather us base it on opinionated items than make it ambiguous.
Then I guess RequestNotFound
can go in InvalidParameters
since I don't see it being using anywhere else besides for invalid parameters. EpochMismatch
is more InvalidState
though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Let's do the follow up for RUSTSEC-2024-0357 |
Terraform Feature Environment Destroy (dev-757)Terraform Initialization ⚙️
|
Flat and opaque way to handle errors.
@ChaoticTempest This approach allowed us to move the errors between categories and remove all error duplications. Now I like it more :)