Skip to content

Add and Apply rust-bitcoin's Lints - #226

Open
luisschwab wants to merge 1 commit into
bitcoindevkit:masterfrom
luisschwab:chore/add-lints
Open

Add and Apply rust-bitcoin's Lints#226
luisschwab wants to merge 1 commit into
bitcoindevkit:masterfrom
luisschwab:chore/add-lints

Conversation

@luisschwab

Copy link
Copy Markdown
Member

This PR adds and applies all of rust-bitcoin's clippy lints and a few useful rust lints.

@luisschwab luisschwab self-assigned this Jul 6, 2026
@luisschwab
luisschwab requested a review from oleonardolima as a code owner July 6, 2026 23:29
@luisschwab

Copy link
Copy Markdown
Member Author

ping @oleonardolima

Comment thread src/lib.rs
Comment on lines 187 to +201
@@ -194,7 +196,9 @@ pub fn convert_fee_rate(target_blocks: usize, estimates: HashMap<u16, FeeRate>)
}

/// Convert a [`HashMap`] of fee estimates expressed as sat/vB ([`f64`]) into [`FeeRate`]s.
pub fn sat_per_vbyte_to_feerate(estimates: HashMap<u16, f64>) -> HashMap<u16, FeeRate> {
pub fn sat_per_vbyte_to_feerate<S: BuildHasher>(
estimates: HashMap<u16, f64, S>,
) -> HashMap<u16, FeeRate> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implicit_hasher = "warn" lint enforces this. It doesn't really make that big of a difference, but allows the caller to pass a HashMap built with any BuildHasher implementation instead of forcing the default one.

Comment thread Cargo.toml
Comment on lines +101 to +103
# Copied over from `rust-bitcoin`
[lints.clippy]
# Exclude lints we don't think are valuable.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need all of them ? Can't we have these in a clippy.toml instead ?

@luisschwab luisschwab Jul 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good for code quality, these have been curated by the rust-bitcoin project over a long time, and they're sensible in my opinion.

I'd rather keep them on Cargo.toml.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer putting them in clippy.toml also since there are so many and it clutters up the Cargo.toml file.

@luisschwab luisschwab Jul 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@notmandatory and @oleonardolima actually, these cannot be on clippy.toml and must be on Cargo.toml; see https://doc.rust-lang.org/stable/clippy/configuration.html#lints-section-in-cargotoml.

error: error reading Clippy's configuration file: unknown field `implicit_clone`, expected one of
           absolute-paths-allowed-crates
           absolute-paths-max-segments
           accept-comment-above-attributes
           accept-comment-above-statement
           [...]
           warn-on-all-wildcard-imports
           warn-unsafe-macro-metavars-in-private-macros
  --> /user/git/rust-esplora-client/clippy.toml:33:1
   |
33 | implicit_clone = "warn"
   | ^^^^^^^^^^^^^^

rust-bitcoin's lints are also setup in the same manner as this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants