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

feat: add response::Error #911

Closed
wants to merge 1 commit into from
Closed

Conversation

npmccallum
Copy link
Contributor

This type makes for efficient use of the ? operator when in a function
with multiple return error types that all implement IntoResponse.

Signed-off-by: Nathaniel McCallum nathaniel@profian.com

This type makes for efficient use of the `?` operator when in a function
with multiple return error types that all implement `IntoResponse`.

Signed-off-by: Nathaniel McCallum <nathaniel@profian.com>
@jplatte
Copy link
Member

jplatte commented Apr 6, 2022

I like the idea, but why are there so many separate From impls instead of a single generic one (impl<T: IntoResponse> From<T> for Error)?

@npmccallum
Copy link
Contributor Author

@jplatte It conflicts with the blanket impl From in the standard library.

@jplatte
Copy link
Member

jplatte commented Apr 6, 2022

Ah, that's unfortunate. I wonder whether there's a way around it 🤔

@npmccallum
Copy link
Contributor Author

Perhaps we could have Error not implement IntoResponse and instead impl IntoResponse for Result<T, Error>?

@davidpdrsn
Copy link
Member

I think this is an interesting idea as well!

Can you give some examples of how you imagine people would use it? Both for error types that you own (and can implement IntoResponse for) and types you don't (like anyhow).

Specifically it needs to beat .map_err(IntoResponse::into_response)? to be justified imo.

@npmccallum
Copy link
Contributor Author

@davidpdrsn We have, for example, different traits for different kinds of backends doing different functions. Each of them have their own error types, all of which impl IntoResponse. However, because there isn't one mega error type (it isn't appropriate for our usage) it means the ? operator doesn't work and we have to do .map_err() all over the place. This causes the flow of our (complex) logic to get hidden between the error-handling overhead.

Therefore, we need one common Error type which can work with the ? operator.

@davidpdrsn
Copy link
Member

So do you then implement From<Error> for each of your error types and return Result<_, Error> from handlers?

@npmccallum
Copy link
Contributor Author

@davidpdrsn No. The goal is that each distinct Error type implements IntoResponse. Then everything should just magically combine into this new Error type.

@npmccallum
Copy link
Contributor Author

I have added a separate PR for a simpler approach. I'll leave this here so we can compare approaches.

@davidpdrsn
Copy link
Member

I think we should go with #921 instead as its simpler and more flexible. I'll close this for now.

@davidpdrsn davidpdrsn closed this Apr 17, 2022
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.

None yet

3 participants