Fix clippy warnings #140
Fix clippy warnings #140
Conversation
…f all crates due to possible public API changes
There was a problem hiding this comment.
I can't guarantee that my review was complete. I might revisit it and add more comments, later. This would be easier if you separated
- style changes
- SemVer relevant changes as part of naming
- functional changes such as Pkce's error
The first category is always welcome, if you decide to split the PR then we can bring this quickly and in the process reduce the diff for all remaining changes.
| @@ -27,7 +27,7 @@ | |||
| //! [`endpoint`]: ../endpoint/index.html | |||
| //! [`Endpoint`]: ../endpoint/trait.Endpoint.html | |||
|
|
|||
| pub mod accesstoken; | |||
| pub mod access_token; | |||
There was a problem hiding this comment.
We can offer a transitional deprecation by adding an alias mod first:
#[deprecated = "Switch to the new spelling, access_token"]
pub mod accesstoken {
pub use super::access_token::*;
}There was a problem hiding this comment.
I'm dragging my feet on this a small bit since breaking changes seem a bit unfortunate for pure warnings. I'd really rather the breaking changes were a separate PR; and there'd be one last as cleaned-up as possible release. Consider the main goal being security and that updates will reach downstream slowly, the best we can do to speed it up is adding good migration helps but given that the crate was essentially stable for over a year I wouldn't expect fast downstream updates either.
In particular, I aim to have a migration guide covering all breaking changes and it'd be easier to get into the details with separate changes.
Specific actions:
- Factoring out the
PkceErrorchange to a separate PR - Providing a deprecated and hidden alias for
mod accesstoken - Clarity on the two lifetime changes
- Providing a hidden, deprecated, temporary alias for the wrong spelling
privileged_to
| pub fn registrar(&self) -> LockResult<MutexGuard<impl Registrar + Send + 'static>> { | ||
| self.registrar.lock() | ||
| } | ||
|
|
||
| /// Thread-safely access the underlying authorizer, which builds and holds authorization codes. | ||
| pub fn authorizer(&self) -> LockResult<MutexGuard<Authorizer + Send + 'static>> { | ||
| pub fn authorizer(&self) -> LockResult<MutexGuard<impl Authorizer + Send + 'static>> { | ||
| self.authorizer.lock() | ||
| } | ||
|
|
||
| /// Thread-safely access the underlying issuer, which builds and holds access tokens. | ||
| pub fn issuer(&self) -> LockResult<MutexGuard<Issuer + Send + 'static>> { | ||
| pub fn issuer(&self) -> LockResult<MutexGuard<impl Issuer + Send + 'static>> { | ||
| self.issuer.lock() | ||
| } |
There was a problem hiding this comment.
I believe these had, and should retain, the semantics of MutexGuard<dyn …>?
There was a problem hiding this comment.
Then I can replace the impl with dyn.
| @@ -170,7 +172,7 @@ impl Authorization { | |||
| } | |||
|
|
|||
| /// Go to next state | |||
| pub fn advance<'req>(&mut self, input: Input<'req>) -> Output<'_> { | |||
| pub fn advance(&mut self, input: Input) -> Output { | |||
There was a problem hiding this comment.
I don't actually know for certain if this is breaking. At face value it looks like this is actually exactly the same. Reference
- Each elided lifetime in the parameters becomes a distinct lifetime parameter.
- If the receiver has type &Self or &mut Self, then the lifetime of that reference to Self is assigned to all elided output lifetime parameters.
| _ => true, | ||
| } | ||
| } | ||
|
|
||
| /// Determines if this scope has enough privileges to access some resource requiring the scope | ||
| /// on the right side. This operation is equivalent to comparison via `>=`. | ||
| pub fn priviledged_to(&self, rhs: &Scope) -> bool { | ||
| pub fn privileged_to(&self, rhs: &Scope) -> bool { |
There was a problem hiding this comment.
Breaking, but can also be solved with a hidden/deprecated method. Weirdly, I have a faint memory of removing this at some point but it may have been somewhere else.
| @@ -27,7 +27,7 @@ | |||
| //! [`endpoint`]: ../endpoint/index.html | |||
| //! [`Endpoint`]: ../endpoint/trait.Endpoint.html | |||
|
|
|||
| pub mod accesstoken; | |||
| pub mod access_token; | |||
There was a problem hiding this comment.
We can offer a transitional deprecation by adding an alias mod first:
#[deprecated = "Switch to the new spelling, access_token"]
pub mod accesstoken {
pub use super::access_token::*;
}
|
I can do that, will work on it tomorrow. |
|
For the unit error warning, I'll just put |
This PR fixes ~40+ clippy warnings and documentation typos in the main crate, along with many others in the subsequent crates for those using
oxide-auth.This also bumps oxide-auth and all its related crates by 1 minor version to comply with SemVer(lots of
Into -> Froms, possible breaking API changes) and to update the oxide-auth to 0.6I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to chose either at their option.