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 trait IntoResponseHeaders #649

Merged
merged 6 commits into from
Jan 23, 2022
Merged

Conversation

jplatte
Copy link
Member

@jplatte jplatte commented Dec 21, 2021

Motivation

It would be nice to be able to return TypedHeader in the same positions as Headers<_>, but that doesn't work without moving it into axum-core due to coherence and that would be very unfortunate. Also, Headers only lives in axum-core due to the same coherence issues.

Solution

Introduce a new trait IntoResponseHeaders and implement it for HeaderMap and Headers. Move Headers into axum.

Notes

I think this is all of the hard stuff dealt with. It should now be straight-forward to move Headers into axum (done), implement IntoResponseHeaders for TypedHeader and move it into its own module (outside response) / re-export it from the axum crate root like Json and add IntoResponse impls for tuples with more IntoResponseHeaders items.

@jplatte jplatte changed the base branch from main to axum-next December 21, 2021 12:09
@jplatte
Copy link
Member Author

jplatte commented Dec 21, 2021

Due to the new blanket impl of IntoResponse, the error message for it not being implemented now mentions IntoResponseHeaders. Don't have the Rust issue link at hand, but it's a somewhat well-known issue.

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 think this is looking really good! Getting these more or less duplicate impls combined into is a big win imo!

If you rebase/merge latest axum-next I'll do a more detailed review, unless there are more things you wanna fix first.

@davidpdrsn davidpdrsn added this to the 0.5 milestone Dec 27, 2021
@jplatte jplatte marked this pull request as ready for review December 28, 2021 09:40
@jplatte
Copy link
Member Author

jplatte commented Dec 28, 2021

I think it makes sense to deal with the proposed TypedHeader changes and adding more impls of the form (StatusCode, $repeat(impl IntoResponseHeaders, N), impl IntoResponse) in separate PRs.

This one is useful as-is.

@davidpdrsn
Copy link
Member

Sorry for taking so long to review this. Hoping to get to it this weekend.

@jplatte
Copy link
Member Author

jplatte commented Jan 21, 2022

Rebased.

@davidpdrsn davidpdrsn merged commit 5bb8df1 into axum-next Jan 23, 2022
@davidpdrsn davidpdrsn deleted the into-response-headers-2 branch January 23, 2022 16:46
davidpdrsn pushed a commit that referenced this pull request Jan 23, 2022
* Introduce IntoResponseHeaders trait

* Implement IntoResponseHeaders for HeaderMap

* Add impl IntoResponse for impl IntoResponseHeaders

… and update IntoResponse impls that use HeaderMap to be generic instead.

* Add impl IntoResponseHeaders for Headers

… and remove IntoResponse impls that use it.

* axum-debug: Fix grammar in docs

* Explain confusing error message in docs
davidpdrsn pushed a commit that referenced this pull request Jan 23, 2022
* Introduce IntoResponseHeaders trait

* Implement IntoResponseHeaders for HeaderMap

* Add impl IntoResponse for impl IntoResponseHeaders

… and update IntoResponse impls that use HeaderMap to be generic instead.

* Add impl IntoResponseHeaders for Headers

… and remove IntoResponse impls that use it.

* axum-debug: Fix grammar in docs

* Explain confusing error message in docs
davidpdrsn pushed a commit that referenced this pull request Jan 23, 2022
* Introduce IntoResponseHeaders trait

* Implement IntoResponseHeaders for HeaderMap

* Add impl IntoResponse for impl IntoResponseHeaders

… and update IntoResponse impls that use HeaderMap to be generic instead.

* Add impl IntoResponseHeaders for Headers

… and remove IntoResponse impls that use it.

* axum-debug: Fix grammar in docs

* Explain confusing error message in docs
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