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

Add response::ErrorResponse and response::Result #921

Merged
merged 8 commits into from
Apr 21, 2022

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

@npmccallum
Copy link
Contributor Author

npmccallum commented Apr 8, 2022

This PR is similar to #911 but is simpler. Note that to achieve this simplicity, Error must not implement IntoResponse. Otherwise, we collide with other blanket implementations.

@davidpdrsn
Copy link
Member

davidpdrsn commented Apr 8, 2022

Can this be defined outside of axum-core, perhaps axum-extra? I would like to avoid expanding its API unless necessary.

@jplatte
Copy link
Member

jplatte commented Apr 8, 2022

I'm pretty sure this can't be defined elsewhere, that IntoResponse impl would violate the orphan rule.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

I really like this, but if we decide to have it, it would be good do have more docs before merging.

I also wonder whether this should have a more specific name and/or whether there should be a Result alias for it.

@npmccallum
Copy link
Contributor Author

@jplatte I like the idea of a Result alias. I don't have strong opinions about the name. However, having Error with an associated Result alias is a pretty common Rust idiom.

Can you tell me what additional docs you're looking for?

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 8, 2022

Before putting more effort into this, I think it makes sense to hear what David thinks again.

In terms of docs, I am thinking of

  • mentioning this type / the Result alias if it's added in the main crate's docs
  • adding an example to the type's docs

@npmccallum
Copy link
Contributor Author

@jplatte I already added an example in the Result alias docs. I'll wait for @davidpdrsn before making any new pushes.

@davidpdrsn
Copy link
Member

I think it's looking good but I wanna play around with it a bit first to see. I'll do that during the weekend and let you know 😊

@npmccallum
Copy link
Contributor Author

@davidpdrsn Any update?

@davidpdrsn
Copy link
Member

davidpdrsn commented Apr 13, 2022

Not yet sorry. I'll let you know.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

I personally prefer having a single error enum made with thiserror and a manual implementation of IntoResponse but for more adhoc stuff this might be nice.

I think the name Error is too broad though, it also conflicts with axum::Error. What about ErrorResponse or ResponseForError?

axum-core/src/response/mod.rs Outdated Show resolved Hide resolved
axum-core/src/response/mod.rs Outdated Show resolved Hide resolved
axum-core/src/response/mod.rs Outdated Show resolved Hide resolved
axum-core/src/response/mod.rs Outdated Show resolved Hide resolved
axum-core/src/response/mod.rs Outdated Show resolved Hide resolved
@jplatte
Copy link
Member

jplatte commented Apr 17, 2022

I like the suggestions of ErrorResponse, in which case maybe the result alias should also have a more unique name (ResultResponse)?

@npmccallum
Copy link
Contributor Author

@jplatte @davidpdrsn I'll leave the decision up to you guys. However, ErrorResponse and ResultResponse seem very non-Rust idiomatic to me. The Rust idiom is to name types Error and Result within their own module. For example: std::io::Error and std::io::Result.

@jplatte
Copy link
Member

jplatte commented Apr 20, 2022

The thing is that ErrorResponse isn't really an error in the traditional sense (it doesn't implement std::error::Error). The fact that ResultResponse is a type alias for Result<_, ErrorResponse> is basically an implementation detail necessary to make ? work currently – we could change this in a breaking release once the try-related traits are stable and can be implemented for thirdparty types.

@davidpdrsn
Copy link
Member

I'm updating this branch now in case you didn't notice. Hopefully I haven't overwritten anything you've done locally.

You're right about std having multiple error types but I see that as kind of a special case, and honestly not very user friendly. I have heard from several users who have been confused about types with the same names, even though they were in different modules. Json for example used to two types. One that was an extractor and one that was a response. That was changed in axum 0.2 I believe.

The error types in std also implement std::error::Error which makes them more error-like I suppose. This error type wouldn't implement that trait.

@npmccallum
Copy link
Contributor Author

My intuition is that less than half of all types used as an error in Result within the Rust ecosystem actually implement std::error::Error (there are a variety of reasons for this, many of which caused the creation of crates like anyhow and thiserror). So I don't see that as a reason to break the idiom here.

And while I agree there is a minor paper cut having two types with the same name in two different modules, I think the greater sin is diverging from ecosystem-wide convention. Anyone who learns Rust will hit the Result naming convention pretty immediately. Diverging from that pattern therefore feels weird to me.

@davidpdrsn
Copy link
Member

I think keeping the Result name is probably fine, thats quite common. But I think we should stick with ErrorResponse.

@jplatte
Copy link
Member

jplatte commented Apr 20, 2022

I don't care too much either way.

@npmccallum
Copy link
Contributor Author

I'm good with the above compromise.

@davidpdrsn
Copy link
Member

Sweet! I'm fixing up the PR now 😊

@npmccallum
Copy link
Contributor Author

I am too. :)

@npmccallum
Copy link
Contributor Author

@davidpdrsn I'll let you fix the PR since you know how you want it.

@npmccallum npmccallum force-pushed the error2 branch 2 times, most recently from 8a9d738 to 65b6793 Compare April 20, 2022 16:41
@npmccallum
Copy link
Contributor Author

I have rebased and made the discussed changes.

@npmccallum
Copy link
Contributor Author

@davidpdrsn I may have accidentally done a force push seconds after your push.

@davidpdrsn
Copy link
Member

Why 😭 I had made a bunch of nitpicky docs changes that I might just force push on top of yours then.

@npmccallum
Copy link
Contributor Author

@davidpdrsn I'm so sorry! Do you still have the commit locally?

@davidpdrsn
Copy link
Member

Yep! I'll look at it tomorrow 😊

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

image

@davidpdrsn davidpdrsn requested a review from jplatte April 21, 2022 07:57
@davidpdrsn davidpdrsn changed the title feat: add response::Error Add response::ErrorResponse and response::Result Apr 21, 2022
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Small nitpick about the changelogs, otherwise looks great!

axum-core/CHANGELOG.md Outdated Show resolved Hide resolved
axum/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
@davidpdrsn davidpdrsn merged commit 8084b24 into tokio-rs:main Apr 21, 2022
@davidpdrsn
Copy link
Member

@npmccallum Thanks for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants