Add and apply rust-bitcoin's lints - #226
Conversation
47bef2f to
f9ae425
Compare
f9ae425 to
80feb72
Compare
| @@ -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> { | |||
There was a problem hiding this comment.
Is this really needed ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Since these functions only consume the map via into_iter(), could we accept impl IntoIterator<Item = (u16, FeeRate)> instead? This also satisfies implicit_hasher, supports more collection types, and avoids exposing BuildHasher in the public API.
| # Copied over from `rust-bitcoin` | ||
| [lints.clippy] | ||
| # Exclude lints we don't think are valuable. |
There was a problem hiding this comment.
Do we really need all of them ? Can't we have these in a clippy.toml instead ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'd prefer putting them in clippy.toml also since there are so many and it clutters up the Cargo.toml file.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Do any of these lints actually trigger errors in our code? If not I'd just add them as/if needed.
There was a problem hiding this comment.
Yes, many of them. I'd rather keep everything to stay consistent with the rest of the rust-bitcoin ecosystem. I think they're sensible, and have been curated by the rust-bitcoin project over a long time.
There was a problem hiding this comment.
Also useful to catch any regressions introduced by new code.
luisschwab
changed the title
rust-bitcoin's Lintsrust-bitcoin's lints
|
@oleonardolima can we get this merged to get the ball rolling? I'd like to get the WASM issue fixed before we cut |
4 participants
This PR adds and applies all of
rust-bitcoin'sclippylints and a few usefulrustlints.