Skip to content

Enable a ton of pedantic clippy lints #228

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

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

apoelstra
Copy link
Member

There are a ton of commits here but they're (mostly) all small and mechanical.

apoelstra added 28 commits June 26, 2025 02:24
Convert a bool to 64 with `from` rather than a manual conversion.
This one is about casts that are totally unnecessary.
As a matter of style, I don't like Default::default because it creates
an object without specifying what type it has.
This one has a huge diff, and there's still a ton of work to do to clean
up these docs, but it should be a big improvement.
This one is a bit silly -- it complains if you do `if !x {} else {}`
because it would supposedly be simpler to swap the if and the else.

In fact, everywhere it triggers in this crate it *would* be an
improvement, or at least neutral. So turn it on. In some cases we are
using if-statements where we could instead use matches.
Just one test. Arguably I should've just whitelisted it. BUt I cleaned
it up.
There is a `map_or` method that encapsulates the `.map().unwrap_or()`
pattern. Use it to reduce the number of function calls the reader has to
read.
This has some API-breaking changes but they're worthwhile (and eliminate
a bunch of clones even in this codebase).
Essentially just a style thing; I think it's nicer to be explicit about
what type we're calling methods on, and I think it's nicer to avoid the
closure syntax when we can.
I'm not convinced that any of these are really necessary, but they don't
hurt and might save somebody a bug.
Just style, and apologies for the annoying-to-review diff that moves
logic around. But the result is more consistent and (I think) better.
Neat! I didn't know about this syntax. Simplifies a lot of opcode
matching.
I think this is a style improvement.
@apoelstra
Copy link
Member Author

cc @canndrew can you review this?

Copy link
Member Author

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

On 57c81d3 successfully ran local tests

@canndrew
Copy link

ACK 57c81d3

Copy link
Member

@delta1 delta1 left a comment

Choose a reason for hiding this comment

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

utACK 57c81d3

@delta1
Copy link
Member

delta1 commented Jun 27, 2025

@apoelstra does the readme MSRV need to be updated? Can also be done in a follow-up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants