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

Impl From<HeaderName> for String #922

Closed

Conversation

matthiasbeyer
Copy link
Contributor

Same as #921 - maybe there's a good reason why this isn't implemented. If so, it is not documented.

😄

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@caspervonb
Copy link
Collaborator

This one might need some discussion, while it is not the case right now (we migrated away from http::header due to casing issues) we do ultimately want to not have any allocations for known headers.

e.g the internal representation is more likely to be something ala:

#[derive(Debug, Clone, Eq, PartialEq, Hash)]
enum Repr<T = ByteString> {
    Standard(StandardHeader),
    Custom(T),
}

@matthiasbeyer
Copy link
Contributor Author

In this case it might still be an idea to provide some to_string() implementation for users, if they want to go the extra step and allocate a String, won't it?

@caspervonb
Copy link
Collaborator

caspervonb commented Apr 13, 2023

So implement Display? (And get blanket ToString implementation).

@matthiasbeyer
Copy link
Contributor Author

I don't know. Is this something that can be shown to a user of an application (or a developer that does some network debugging or whatever)?

And can ByteString (not sure whether it would come from) implement Display as well?

Maybe AsRef<str> would be enough...

@caspervonb
Copy link
Collaborator

caspervonb commented Apr 16, 2023

And can ByteString (not sure whether it would come from) implement Display as well?

It can, semantics are just a bit different (imho). Into implies clean conversion, where-as ToString implies "get there in some way".

Did fmt::Display over in #924 which provides ToString.

Don't think we should provide a blanket into string implementation for HeaderName at this time. Feel free to rebut me on this tho.

@matthiasbeyer
Copy link
Contributor Author

Closing as superseded by #924

@caspervonb 👍 thanks!

@matthiasbeyer matthiasbeyer deleted the header-name-into-string branch April 16, 2023 17:49
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