Fix clippy warnings#241
Conversation
|
Concept ACK, you've probably missed something in |
As directed by clippy use `unwrap_or_else` in order to take advantage of lazy evaluation.
As directed by clippy use `!a.is_empty()` instead of `a.len() > 0`.
Its more idiomatic to implement `Default` instead of a constructor with no args `new`. Found by clippy.
const str types do not need an explicit lifetime, remove it. Found by clippy.
Clippy emits warning: warning: manual `!RangeInclusive::contains` implementation Use `RangeInclusive::contains` as suggested.
Clippy emits warning about unnecessary wraps. If a function never fails no need to return a `Result`. In the case where we require a function signature that includes an error return use `allow` attribute.
No need to clone copy types, found by clippy.
Clippy emits warning: warning: needlessly taken reference of both operands Remove the explicit reference's as suggested.
Clippy warns a bunch of times due to float comparison. All are within unit tests and we test against values with one single zero in the decimal place. We can safely convert to `u32` within the assertion.
Clippy emits warning: warning: avoid using `collect()` when not needed As suggested by clippy just use `count` directly on the iterator instead of `collect` followed by `len`.
As suggested by clippy we can use `.next()` on an iterator instead of `nth(0)`. Although arguably no clearer, and certainly no worse, it keeps clippy quiet and a clean lint is a good thing.
Clippy emits warning:
warning: field assignment outside of initializer for an instance
created with Default::default()
Do as suggested by clippy and use the default init pattern.
```
let foo = Foo {
bar: ...,
Default::default()
}
```
Clippy emits warning:
warning: the `err @ _` pattern can be written as just `err`
As suggested by clippy remove the `@ _`.
Note: This patch contains Cargo cult programming. I do not actually
understand what this patter is meant to do, blindly following clippy.
Functions that never err do not need to return and error type. Remove the unnecessary error type and fix all the call sites.
Clippy emits warning:
warning: this `.into_iter()` call is equivalent to `.iter()` and
will not consume the `Vec`
Use `iter` as suggested by clippy.
Manually matching can be replace with the `matches!` macro. Found by clippy.
Redundant closure can be removed as suggested by clippy.
Clippy warns due to manual implementation of saturating subtraction. As suggested by clippy use `saturating_sub` instead.
Calling a function within `unwrap_or` is sub-optimal because the function is always evaluated. By using `unwrap_or_else` we get lazy evaluation. Found by clippy.
Clippy complains about use of a mutex, suggesting we use an `AtomicUsize`. While the same functionality _could_ be achieved using an `AtomicUsize` and a CAS loop it makes the code harder to reason about for little gain. Lets just quieten clippy with an allow attribute and document why we did so.
The `ChunksIterator` constructor is only used when either `electrum` or `esplora` features are enabled. Conditionally build it so that we do not get a clippy warning when building without these features.
Remove the TODO; refactor matching to correctly handle conditionally built `Sled` variants. Use `unreachable` instead of `unimplemented` with a comment hinting that this is a bug, this makes it explicit, both at runtime and when reading the code, that this match arm should not be hit.
tcharding
force-pushed
the
126-clippy
branch
from
94939f1 to
38f1a55
Compare
Clippy emits error: comparing trait object pointers compares a non-unique vtable address The vtable is an implementation detail, it may change in future. we should not be comparing vtable addresses for equality. Instead we can get a pointer to the data field of a fat pointer and compare on that.
tcharding
force-pushed
the
126-clippy
branch
from
38f1a55 to
e713520
Compare
|
I think most of these fixes are for the |
| ); | ||
| assert!(matches!(signers.find(id1), Some(signer) if is_equal(signer, &signer1))); | ||
| assert!(matches!(signers.find(id2), Some(signer) if is_equal(signer, &signer2))); | ||
| assert!(matches!(signers.find(id3.clone()), Some(signer) if is_equal(signer, &signer3))); |
There was a problem hiding this comment.
❓ Can't we just make dyn Signer Eq?
@afilini is there some coherent way we can identify a signer i.e. by it's xprv/xpub or something more clever?
There was a problem hiding this comment.
Yeah, makes sense. I would add an .id() method to Signer that returns a SignerId. Then we can implement Eq by checking the id returned by both signers.
Let me open an issue for this
Ok, I think I need to learn how to correctly set up my dev environment for new projects before attempting to contribute :)
Closing for now.
Thanks for schooling me. This was not the only project where I was wondering why so many clippy warnings were there on master - turns out its me :( I will pull out the relevant changes and re-open a new PR later on. Sorry for the noise. |
|
Hey no problem, rust tooling is great but does have it's own learning curve to deal with. For this project we run our CI build/test pipeline against minimum version |
4 participants
Description
As described in issue #126 we have a bunch of clippy warnings. This PR clears the warnings.
Notes to the reviewers
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.mdBugfixes:
Fixes: #126
(I think this resolves the issue, the CI description looks stale but perhaps I'm misunderstanding it, please review the issue description to see if I have missed anything).
Thanks