Clean up clippy allows by ValuedMammal · Pull Request #1186 · bitcoindevkit/bdk

ValuedMammal · 2023-10-29T23:22:39Z

There are several instances in the code where we allow clippy lints that would otherwise be flagged during regular checks. It would be preferable to minimize the number of "clippy allow" attributes either by fixing the affected areas or setting a specific configuration in clippy.toml. In cases where we have to allow a particular lint, it should be documented why the lint doesn't apply.

For context see #1127 (comment) as well as the commit message details.

One area I'm unsure of is whether Boxing a large error in 4fc2216 is the right approach. Logically it makes sense to avoid allocating a needlessly heavy Result, but I haven't studied the implications or tradeoffs of such a change.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

ValuedMammal · 2023-10-31T17:14:07Z

The first attempt failed CI due to clippy::iter_nth_zero. Of course, there's no reason to think the lint could be disregarded in the first place.. I believe I used the wrong invocation of cargo clippy on the command line, allowing it to slip by. I suppose the nix experience should eventually help prevent silly mistakes.

After taking another look at spk_iter.rs, I think it's fine to allow the exception (as it was originally), since we're specifically testing SpkIterator's custom implementation of nth. An alternative would be to write the tests in a way that doesn't require calling nth(0), but for now this seems like a trivial case not worth worrying about.

nursery/tmp_plan/src/requirements.rs

danielabrozzoni

Overall it looks really good, thanks a lot!
A few questions/suggestions:

example-crates/example_cli/src/lib.rs

crates/esplora/src/async_ext.rs

crates/bdk/src/wallet/coin_selection.rs

ValuedMammal · 2023-11-10T17:11:42Z

rebasing now. dropped all the changes to coin_selection.rs, and made ref(tmp_plan) the last commit

ValuedMammal · 2023-11-10T17:38:19Z

crates/bdk/src/wallet/mod.rs

I guess I'm on the fence regarding preselect_utxos. @LLFourn would you prefer we left this untouched so it's clear at the call site exactly what information we're passing?

ValuedMammal · 2023-12-17T06:15:16Z

Reminder to bump msrv in clippy.toml

warning: the MSRV in `clippy.toml` and `Cargo.toml` differ; using `1.57.0` from `clippy.toml`

ValuedMammal · 2024-01-01T20:49:02Z

Changes in most recent push:

re-refactored Psbt::get_utxo_for to use a match block. I think it conveys the same thing in a more concise way. compare

nursery/tmp_plan/src/lib.rs

LLFourn · 2024-01-03T03:48:43Z

Please revert all changes to the tmp plan crate and just blanket allow everything. Other than that this looks good to go.

ValuedMammal · 2024-01-03T04:38:57Z

@LLFourn appreciate your input, thanks for clarifying.

example-crates/example_cli/src/lib.rs

LLFourn

ACK c4f1211

Thanks @ValuedMammal!

ValuedMammal · 2024-01-24T18:03:49Z

Is anyone opposed to making the type InitialState in example_cli into a struct with comments?

/// The initial state returned from [`init`].
pub struct Init<CS: clap::Subcommand, S: clap::Args, C> {
    /// Arguments parsed by the cli.
    pub args: Args<CS, S>,
    /// Descriptor keymap.
    pub keymap: KeyMap,
    /// Keychain-txout index.
    pub index: KeychainTxOutIndex<Keychain>,
    /// Persistence backend.
    pub db: Mutex<Database<C>>,
    /// Initial changeset.
    pub init_changeset: C,
}

danielabrozzoni · 2024-01-30T21:43:22Z

@ValuedMammal go for it!

These lints either resolved themselves, or the code has changed such that they no longer apply, hence they can be removed with no further changes. `clippy::derivable_impls` `clippy::needless_collect` `clippy::almost_swapped`

evanlinjin

Looking good! Just a couple of NITs

crates/bdk/src/psbt/mod.rs

example-crates/example_cli/src/lib.rs

- Add typedefs to model the result of functions `planned_utxos` and `init` - Add new struct `CreateTxChange` to hold any change info resulting from `create_tx` These changes help resolve clippy::type_complexity

…_map`

to reduce the number of required function args in order to satisfy `clippy::too_many_arguments`

to address `clippy::result_large_err`. Clippy's default large-error- threshold is 128. `esplora_client::Error` currently has size 272.

for holding the items returned from `example_cli::init`

ValuedMammal · 2024-01-31T17:07:10Z

Rebased onto f099b42
Addressed comments by @evanlinjin

Thank you to all the reviewers! @danielabrozzoni @LLFourn

evanlinjin

ACK 1c15cb2

ValuedMammal changed the title ~~Clippy~~ Clean up clippy allows Oct 29, 2023

ValuedMammal force-pushed the clippy branch from 06d2f20 to 4fc2216 Compare October 30, 2023 22:36

LLFourn reviewed Nov 2, 2023

View reviewed changes

nursery/tmp_plan/src/requirements.rs Outdated Show resolved Hide resolved

ValuedMammal force-pushed the clippy branch from 4fc2216 to 17d9ede Compare November 3, 2023 13:42

nondiremanuel added this to the 1.0.0-alpha.3 milestone Nov 5, 2023

nondiremanuel mentioned this pull request Nov 5, 2023

W45 Lib Team Call bitcoindevkit/.github#84

Closed

38 tasks

ValuedMammal force-pushed the clippy branch from 17d9ede to f90d696 Compare November 6, 2023 15:36

danielabrozzoni reviewed Nov 9, 2023

View reviewed changes

example-crates/example_cli/src/lib.rs Outdated Show resolved Hide resolved

crates/esplora/src/async_ext.rs Outdated Show resolved Hide resolved

danielabrozzoni reviewed Nov 10, 2023

View reviewed changes

crates/bdk/src/wallet/coin_selection.rs Outdated Show resolved Hide resolved

ValuedMammal force-pushed the clippy branch from f90d696 to 0ba1e94 Compare November 10, 2023 17:07

ValuedMammal commented Nov 10, 2023

View reviewed changes

notmandatory assigned ValuedMammal Nov 12, 2023

ValuedMammal force-pushed the clippy branch from 0ba1e94 to ee6e76b Compare November 14, 2023 00:47

notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 15, 2023

ValuedMammal force-pushed the clippy branch from ee6e76b to 6cdc7f1 Compare November 17, 2023 03:59

nondiremanuel mentioned this pull request Nov 19, 2023

W47 Lib Team Call bitcoindevkit/.github#86

Closed

54 tasks

nondiremanuel mentioned this pull request Dec 2, 2023

W49 Lib Team Call bitcoindevkit/.github#87

Closed

55 tasks

ValuedMammal force-pushed the clippy branch from 6cdc7f1 to 6107725 Compare December 29, 2023 19:54

LLFourn reviewed Jan 3, 2024

View reviewed changes

nursery/tmp_plan/src/lib.rs Outdated Show resolved Hide resolved

ValuedMammal force-pushed the clippy branch from 6107725 to 9003b76 Compare January 3, 2024 04:38

nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024

ValuedMammal force-pushed the clippy branch from 9003b76 to 4509023 Compare January 8, 2024 20:19

ValuedMammal force-pushed the clippy branch from 4509023 to c4f1211 Compare January 21, 2024 17:26

LLFourn reviewed Jan 22, 2024

View reviewed changes

example-crates/example_cli/src/lib.rs Outdated Show resolved Hide resolved

LLFourn approved these changes Jan 22, 2024

View reviewed changes

refactor: remove old clippy allow attributes

64a9019

These lints either resolved themselves, or the code has changed such that they no longer apply, hence they can be removed with no further changes. `clippy::derivable_impls` `clippy::needless_collect` `clippy::almost_swapped`

ValuedMammal force-pushed the clippy branch from d66dfd8 to 61cda41 Compare January 31, 2024 16:23

evanlinjin reviewed Jan 31, 2024

View reviewed changes

crates/bdk/src/psbt/mod.rs Outdated Show resolved Hide resolved

example-crates/example_cli/src/lib.rs Outdated Show resolved Hide resolved

ValuedMammal added 5 commits January 31, 2024 11:50

ref(example_cli): add typedefs to reduce type complexity

4679ca1

- Add typedefs to model the result of functions `planned_utxos` and `init` - Add new struct `CreateTxChange` to hold any change info resulting from `create_tx` These changes help resolve clippy::type_complexity

ref(psbt): refactor body of get_utxo_for to address `clippy::manual…

f11d663

…_map`

ref(wallet): Wallet::preselect_utxos now accepts a &TxParams

097d818

to reduce the number of required function args in order to satisfy `clippy::too_many_arguments`

ref(esplora): Box a large esplora_client::Error

89a7ddc

to address `clippy::result_large_err`. Clippy's default large-error- threshold is 128. `esplora_client::Error` currently has size 272.

ref(example_cli): Add new struct Init

1c15cb2

for holding the items returned from `example_cli::init`

ValuedMammal force-pushed the clippy branch from 61cda41 to 1c15cb2 Compare January 31, 2024 16:58

evanlinjin approved these changes Jan 31, 2024

View reviewed changes

evanlinjin merged commit c6b9ed3 into bitcoindevkit:master Jan 31, 2024

ValuedMammal deleted the clippy branch February 1, 2024 02:36

Conversation

ValuedMammal commented Oct 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklists

All Submissions:

Uh oh!

ValuedMammal commented Oct 31, 2023

Uh oh!

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValuedMammal commented Nov 10, 2023

Uh oh!

ValuedMammal Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

ValuedMammal commented Dec 17, 2023

Uh oh!

ValuedMammal commented Jan 1, 2024

Uh oh!

Uh oh!

LLFourn commented Jan 3, 2024

Uh oh!

ValuedMammal commented Jan 3, 2024

Uh oh!

Uh oh!

LLFourn left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal commented Jan 24, 2024

Uh oh!

danielabrozzoni commented Jan 30, 2024

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ValuedMammal commented Jan 31, 2024

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ValuedMammal commented Oct 29, 2023 •

edited

Loading