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

Way too many clippy::allow #1127

Closed
Tracked by #78 ...
LLFourn opened this issue Sep 19, 2023 · 4 comments · Fixed by #1186
Closed
Tracked by #78 ...

Way too many clippy::allow #1127

LLFourn opened this issue Sep 19, 2023 · 4 comments · Fixed by #1186
Assignees
Labels
good first issue Good for newcomers
Milestone

Comments

@LLFourn
Copy link
Contributor

LLFourn commented Sep 19, 2023

It seems we've got in the habit of doing clippy::allow at many points that clippy complains about something. I count ~20 places. I think we should fix most these and refrain from doing it in the future (don't merge new PRs with clippy::allow in them).

@apoelstra
Copy link

Alternately (I haven't looked at specific instances here, maybe the right answer is just "fix the code"):

  • Move them to lib.rs with a clear comment explaining why we think the lint is fundamentally not useful;
  • Check if there is a clippy.toml setting we can adjust (e.g. maximum number of function arguments)

@LLFourn
Copy link
Contributor Author

LLFourn commented Sep 19, 2023

Indeed. Just globally disabling certain lints is much better than individually disabling every instance we violate them. If the person taking this issue feels inspired they could look and see if there are any lints that aren't enabled by default that we could enable in clippy.toml to improve code quality.

@ValuedMammal
Copy link
Contributor

After some initial triage, I can provide this for context:

I first removed all the clippy allow attributes and ran cargo clippy -- -Wclippy::all on stable rust 1.67 to reproduce the lints, however not all of them reproduced, indicating some 'allow' macros might have been stale or no longer apply. Good news there, unless I missed something.

The default too-many-arguments-threshold is 7. The main offenders are:

wallet/coin_selection.rs
bnb (9)
single_random_draw (7)

wallet/mod.rs
preselect_utxos (8)

tmp_plan/src/requirements.rs
sign_with_keymap (9)

Some refactoring would help here, or the lazy way would be to simply set too-many-arguments-threshold = 9 in clippy.toml, which would give you 8 function params plus a self.

The main complaint in esplora/{async,blocking}_ext.rs is clippy::result_large_err referring to the size of esplora_client::Error. The default large-error-threshold is 128, and the error in question has size 272. Clippy suggests Boxing the error.

Similar to the large error lint, enum PlanState<Ak> in tmp_plan/src/lib.rs triggers clippy::large_enum_variant. The larger enum variant, Incomplete(Requirements<Ak>) is at least 433 bytes according to clippy, and the next largest variant is only 64 bytes. I'm not if sure this one needs special attention, maybe just a code comment.

I started work on a branch that addresses the issues and brings the number of 'allow' macros down to 2 without changing clippy.toml, however a combination of code changes and clippy config tweaks might be warranted.

@LLFourn
Copy link
Contributor Author

LLFourn commented Oct 30, 2023

in tmp_plan we can allow everything (it is only used in examples and will be removed soon).
We can also allow everything in coin_selection.rs since it will soon go away too.
I think preselect_utxos can be allow because it's private (it should also go away some time next year).

So I think we only need to fix the esplora ones.

@notmandatory notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 13, 2023
@notmandatory notmandatory moved this from Todo to Needs Review in BDK Nov 15, 2023
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants