hyperium / headers

Typed HTTP Headers from hyper
https://hyper.rs
MIT License
162 stars 84 forks source link

Implement Deref to C for Authorization<C> #100

Closed jplatte closed 2 years ago

jplatte commented 2 years ago

I had to undo automatic formatting a few times, I would recommend you run cargo fmt with a recent toolchain so this doesn't happen to other contributors.

jplatte commented 2 years ago

Ping @seanmonstar

seanmonstar commented 2 years ago

Hm, usually Deref is reserved for "smart pointers". The Authorization type doesn't seem like one.

davidpdrsn commented 2 years ago

I think its fine since Authorization is a tuple struct with a single public field and doesn't have any methods (only two constructors). We do something similar in axum on all the tuple structs we have there.

jplatte commented 2 years ago

I think it's common enough for types that aren't really smart pointers to implement Deref. Like in axum, actix-web's extractor types like web::Json also implement Deref to the inner type. SQLx'es Transaction type deref's to a Connection.

jplatte commented 2 years ago

Hey @seanmonstar, how can I move this forward? Would you like to first see some more general discussion about types implementing Deref that aren't smart pointers on the Rust issue tracker or the Rust API guidelines? (I'd guess there's existing issues about it, but not sure)

seanmonstar commented 2 years ago

That'd be fine to link. So far, I'm still not sure this is a case where using Deref is the right pattern.

jplatte commented 2 years ago

Seems like it's pretty clear that the "only smart pointers" rule is wrong, with there being three examples (two stable) of types contradicting it in std:

(from https://github.com/rust-lang/api-guidelines/issues/249)

jplatte commented 2 years ago

@seanmonstar if you're still not (fully) convinced, how about I update this PR to provide impl Authorization<Bearer> and impl Authorization<Basic> with forwarding fn token / fn username / fn password?

seanmonstar commented 2 years ago

That option seems fine.

jplatte commented 2 years ago

Actually just today I wrote some code where I created my own Credentials. I can't implement stuff on Authorization<MyCreds> so am once again wishing for the Deref impl.

Did you look at the issue I linked?