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

Implement Deref to C for Authorization<C> #100

Closed
wants to merge 4 commits into from
Closed

Implement Deref to C for Authorization<C> #100

wants to merge 4 commits into from

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Dec 16, 2021

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 and others added 4 commits December 16, 2021 12:41
Found by clippy.
Allow calling `.token()` on `Authorization<Basic>` and
`Authorization<Bearer>` without destructuring first.
@jplatte
Copy link
Contributor Author

jplatte commented Jan 3, 2022

Ping @seanmonstar

@seanmonstar
Copy link
Member

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

@davidpdrsn
Copy link
Member

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
Copy link
Contributor Author

jplatte commented Jan 3, 2022

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
Copy link
Contributor Author

jplatte commented Jan 12, 2022

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
Copy link
Member

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
Copy link
Contributor Author

jplatte commented Jan 13, 2022

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:

  • ManuallyDrop
  • AssertUnwindSafe
  • Lazy (unstable)

(from rust-lang/api-guidelines#249)

@jplatte
Copy link
Contributor Author

jplatte commented Jan 19, 2022

@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
Copy link
Member

That option seems fine.

@jplatte
Copy link
Contributor Author

jplatte commented Jan 19, 2022

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?

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.

3 participants