-
Notifications
You must be signed in to change notification settings - Fork 4
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
Make the error type configurable #153
Conversation
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.
Copilot reviewed 8 out of 10 changed files in this pull request and generated no comments.
Files not reviewed (2)
- crates/twirp/src/client.rs: Evaluated as low risk
- crates/twirp/src/details.rs: Evaluated as low risk
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
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.
I like this idea a lot. I'm a little unsure about the IntoTwirpResponse
as it's really not doing what it says (and isn't there an axum trait for IntoResponse
already?).
It's still possible to return a response with an invalid status code or headers, but less likely to happen by accident.
c4bbfd5
to
319d675
Compare
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.
Nice!
So far, on the server side, we've required users to use a hard-coded error type,
TwirpErrorResponse
. The rationale was that we need an error type that the twirp machinery can convert into a syntactically valid Twirp response.However:
So this PR makes the error type configurable, requiring the user to implement a new
IntoTwirpResponse
trait to make sure users don't accidentally try to use error types that don't generate valid Twirp responses.