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

CI: Lint the crate and examples #671

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

tcharding
Copy link
Member

We are not currently running the linter in CI, do so.

@tcharding tcharding force-pushed the 04-01-enable-clippy branch from 6f7495d to ea4d30d Compare April 1, 2024 04:26
@apoelstra
Copy link
Member

Will $clippy --all-targets get the same effect?

I think we should try running

clippy --all-features --all-targets
clippy --all-targets
clippy --no-default-features --all-targets # maybe with --features=no-std if we require that here

Which I think will cover the examples (though let me check..) and then gets us a bit of feature-matrix coverage as well.

@apoelstra
Copy link
Member

Yeah, confirmed that --all-targets will flag let x = u8::from(10u8) in an example program.

@tcharding
Copy link
Member Author

tcharding commented Apr 1, 2024

Interesting, then we (I) got it wrong in rust-bitcoin - I thought I had checked this before.

IIUC running the linter with different feature flags only adds checking unused imports when features are turned off, and while its nice to have these correct I'm not sure its worth bothering contributors with failing CI runs for it?

EDIT: In hindsight this comment is dopey as hell :)

@tcharding tcharding force-pushed the 04-01-enable-clippy branch from ea4d30d to 01a8642 Compare April 1, 2024 21:39
@tcharding
Copy link
Member Author

tcharding commented Apr 1, 2024

I was wrong we get real errors with cargo +nightly clippy --no-default-features --features=no-std --all-targets -- -D warnings that we don't get with --all-features - WIN.

Here is an example

error: this expression creates a reference which is immediately dereferenced by the compiler
   --> src/lib.rs:757:37
    |
757 |             fn deref(&self) -> &T { &self.lock.deref() }
    |                                     ^^^^^^^^^^^^^^^^^^ help: change this to: `self.lock.deref()`

clippy emits:

 error: this expression creates a reference which is immediately
 dereferenced by the compiler

As suggested, remove the reference.
clippy emits:

 error: the following explicit lifetimes could be elided: 'a

As suggested, remove the explicit lifetime.
We are not currently running the linter in CI, do so.

Lint with three different feature combinations to get reasonable
coverage.
@tcharding tcharding force-pushed the 04-01-enable-clippy branch from 01a8642 to a410c06 Compare April 1, 2024 21:47
@tcharding
Copy link
Member Author

Now includes two clippy fixes up front and uses the three feature gate combinations suggested.

Copy link
Member

@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.

ACK a410c06

@apoelstra apoelstra merged commit a548edd into rust-bitcoin:master Apr 1, 2024
16 checks passed
@tcharding tcharding deleted the 04-01-enable-clippy branch April 9, 2024 00:44
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.

2 participants