Explore clippy rules and apply quality of life recommendations #122
Conversation
| mod time; | ||
| mod validate; | ||
| mod version; | ||
| mod vote; |
There was a problem hiding this comment.
The disadvantage of relying on re-exports of types defined in non-pub modules is rustc will refer to those types by their fully qualified path regardless of the visibility of the module it's contained in, which leaves someone trying to debug in the bizarre position that the compiler referring to a path which cannot be accessed and does not exist in rustdoc.
That said, I do sometimes do this myself, but it's usually when I'm trying to create a crate where the code is factored into modules internally but all types are exported at the toplevel (and when I do that, it's generally because the external API surface of the crate is small to begin with).
The other disadvantage is this approach eliminates the ability to have module-level documentation visible in the rustdoc.
There was a problem hiding this comment.
What would you propose generally? I think amino_types module organisation is ripe for a review anyway.
There was a problem hiding this comment.
Re-export them, but leave the containing modules pub if they're part of the public API
| @@ -3,7 +3,7 @@ use std::convert::TryInto; | |||
|
|
|||
| /// Extend the original prost::Message trait with a few helper functions in order to | |||
| /// reduce boiler-plate code (and without modifying the prost-amino dependency). | |||
| pub trait AminoMessage: prost_amino::Message { | |||
| pub trait Message: prost_amino::Message { | |||
There was a problem hiding this comment.
👍I'm a fan of eliminating module (and crate) names from type names. Some relevant API guidelines and issues:
Importing modules rather than individual types and referring to types by their module names can also help cut down on the number of imports.
It's hard to avoid multiple crate versions given current transient dependencies, some of them we control and we are following up to bring most of them on par: tendermint/amino_rs#20
| fn from(height: Height) -> Self { | ||
| height.0 as Self | ||
| } | ||
| } |
There was a problem hiding this comment.
Did we verify this isn't used in the KMS for instance?
There was a problem hiding this comment.
Does it matter? Isn't this commit non-breaking?
There was a problem hiding this comment.
Weird, I saw this comment on a different commit: 67f7358#r368718476.
The correct one is 7750986#r368718476.
So yes we should verify we can get rid of this
| Ok(have) => assert_eq!(have, msg), | ||
| Err(err) => panic!(err.to_string()), | ||
| } | ||
| let have = PubKeyRequest::decode(want.as_ref()).unwrap(); |
There was a problem hiding this comment.
Didn't 2 other commits turn all option/result unwraps into expects?
| Ok(have) => assert_eq!(have, msg), | ||
| Err(err) => panic!(err), | ||
| } | ||
| let have = PubKeyResponse::decode(encoded.as_ref()).unwrap(); |
There was a problem hiding this comment.
| /// and trusted validator set is sufficient for a commit to be | ||
| /// accepted going forward. | ||
| /// The default implementation returns true, iff at least a third of the trusted | ||
| /// voting power signed (in other words at least one honest validator signed). | ||
| /// Some clients might require more than +1/3 and can implement their own | ||
| /// TrustLevel which can be passed into all relevant methods. | ||
| /// [`TrustLevel`] which can be passed into all relevant methods. |
There was a problem hiding this comment.
Looks like we forgot to rename TrustLevel to TrustThreshold here. Running cargo +nightly doc --open --no-deps will error now with something like
`[TrustLevel]` cannot be resolved, ignoring it.
because of a #[deny(warnings)].
Does this mean building for https://docs.rs/ won't work now?
There was a problem hiding this comment.
I'm not sure if docs.rs will break if you have intra-doc resolution failures and deny warnings but possibly.
At any rate, it's good to capture and correct these. Running cargo test --doc on nightly will catch them.
| /// but there may be some intersection. The trust_level parameter allows clients to require more | ||
| /// than +1/3 by implementing the TrustLevel trait accordingly. | ||
| /// but there may be some intersection. The `trust_level` parameter allows clients to require more | ||
| /// than +1/3 by implementing the `TrustLevel` trait accordingly. |
There was a problem hiding this comment.
Consistent would be [`TrustThreshold `]. Maybe a follow-up PR could deal solely with linkage in documentation.
There was a problem hiding this comment.
Great work @xla!
I think these two comments require acting in form of replying to the comment or addressing the suggested changes:
https://github.com/interchainio/tendermint-rs/pull/122/files#r369395194
https://github.com/interchainio/tendermint-rs/pull/122/files#r368546339
Everything else looks good to me. As a general comment: even though chunking changes into commits addressing one lint at a time made this easier to review, I would vouch for splitting up a PR like this into separate PRs instead of a single (large one) for the future.
Ideally, I would let @tarcieri give it another pass. AFAIK he's trying to unblock some stuff on the kms. Although this PR looks like it doesn't break anything the kms uses, I would appreciate another pair of eyes here for confirmation in this context.
| @@ -23,6 +23,7 @@ impl Transaction { | |||
| Self(into_vec.into()) | |||
| } | |||
|
|
|||
| #[allow(clippy::missing_const_for_fn)] | |||
| @@ -36,7 +36,13 @@ fn get_split_point(length: usize) -> usize { | |||
| 0 => panic!("tree is empty!"), | |||
| 1 => panic!("tree has only one element!"), | |||
| 2 => 1, | |||
| _ => length.next_power_of_two() / 2, | |||
| _ => { | |||
| let (div, overflow) = length.next_power_of_two().overflowing_div(2); | |||
There was a problem hiding this comment.
Is this really necessary? length and next_power_of_two are both usize which will never overflow ... Same goes for the wrapping_div above
| _ => return Err(err!(ErrorKind::Parse, "invalid units")), | ||
| }; | ||
|
|
||
| let numeric_part = s.chars().take(s.len() - units.len()).collect::<String>(); | ||
| let numeric_part = match units { |
There was a problem hiding this comment.
What does this enum have to do with Rework use of unreachable ?
Also, can the Unit variants contain their own string representation so we don't need a match here like before?
|
Reviewed this in full. Great work but I agree it should be broken up into smaller PRs with loosely themed lints and may need another pass/updates on certain commits. Also we need to pay more attention to breaking changes and track them accordingly. Maybe the smaller PRs for this can also be distinguished by whether or not they're breaking. |
| @@ -19,17 +19,17 @@ pub enum Code { | |||
| } | |||
|
|
|||
| impl Default for Code { | |||
| fn default() -> Code { | |||
There was a problem hiding this comment.
So in general I think this is probably good, but I wonder if in the case of large impl blocks if it would be better to use the explicit type name.
|
@brapse Currently working on incremental changes on top of latest master to superseed this PR. |
|
@xla Can/should we link the new PR to this one and close this? |
|
Let's track in #193 and work on it in the follow ups |
First of, this is an attempt to codify clippy rules. And find the right amount to be applied for consistent code shape. Least it helps me explore the codebase. Will keep inventory which commits are driven by what rule. We can disregard any if too rigid.