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.

Sign up for GitHub

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

Jump to bottom

Add AppendHeaders #927

Merged
merged 3 commits into from
Apr 17, 2022
Merged

Add AppendHeaders #927

merged 3 commits into from
Apr 17, 2022

Conversation

davidpdrsn
Copy link
Member

Fixes #925

@davidpdrsn davidpdrsn requested a review from jplatte April 9, 2022 17:15
jplatte
jplatte reviewed Apr 9, 2022
View reviewed changes
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.

This can be part of axum rather than axum-core, no?

@davidpdrsn
Copy link
Member Author

I thought so at first but the constructors for TryIntoHeaderError are private. So either those have to be made public or this goes into axum-core. I chose the latter but don't feel strongly about it though. Figured since AppendHeaders has a very small API its probably fine.

@davidpdrsn davidpdrsn requested a review from jplatte April 17, 2022 08:15
@jplatte
Copy link
Member

jplatte commented Apr 17, 2022

I see. I think it would be fine to have those constructors public, but not clearly better than having AppendHeaders in axum-core.

@davidpdrsn davidpdrsn enabled auto-merge (squash) April 17, 2022 21:07
@davidpdrsn davidpdrsn disabled auto-merge April 17, 2022 21:14
@davidpdrsn davidpdrsn merged commit afcefb4 into main Apr 17, 2022
@davidpdrsn davidpdrsn deleted the append-headers branch April 17, 2022 21:14
@jtk18
Copy link

jtk18 commented Oct 20, 2022

Why the constant array? Can this be a vec instead? Thank you!

@davidpdrsn
Copy link
Member Author

I don't specifically remember. We probably could use any IntoIterator.

@jtk18
Copy link

jtk18 commented Oct 20, 2022

That would make it much easier to work with. Thank you for the great addition!

@jplatte jplatte mentioned this pull request Oct 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jplatte jplatte jplatte approved these changes

Assignees
No one assigned
Labels
A-axum-core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow response multiple headers with the same name
3 participants
@davidpdrsn @jplatte @jtk18