-
Notifications
You must be signed in to change notification settings - Fork 106
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
Add Clippy to CI #224
Add Clippy to CI #224
Conversation
uses: actions-rs/clippy-check@v1 | ||
with: | ||
token: ${{ secrets.GITHUB_TOKEN }} | ||
args: --all-features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to the existing run only because I wasn't sure whether the caching would work otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure of whether this is the best way to add it.
@@ -31,6 +31,8 @@ use crate::types::{BlockHeight, LockTime}; | |||
/// internally by different enum variants. Because we checkpoint on Sapling | |||
/// activation, we do not parse any pre-Sapling transaction types. | |||
#[derive(Clone, Debug, PartialEq, Eq)] | |||
// XXX consider boxing the Optional fields of V4 txs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -255,10 +256,11 @@ pub enum Message { | |||
// `tx_witnesses` aren't either, as they go if `flag` goes. | |||
Tx { | |||
/// Transaction data format version (note, this is signed). | |||
// XXX do we still need this with the transaction data handling? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -134,10 +134,11 @@ pub enum Message { | |||
/// [Bitcoin reference](https://en.bitcoin.it/wiki/Protocol_documentation#block) | |||
Block { | |||
/// Transaction data format version (note, this is signed). | |||
// XXX does this get folded into the Block struct? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the Clippy "cognitive complexity" lints are a false positive due to Clippy counting code inside of #![allow(clippy::cognitive_complexity)] globally... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
- name: Run clippy | ||
uses: actions-rs/clippy-check@v1 | ||
with: | ||
token: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need the GITHUB_TOKEN?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the Action recommends it but I'm unclear as to why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah the actual posting of check results as annotations requires it I think.
@@ -186,14 +186,14 @@ impl Arbitrary for ShieldedData { | |||
vec(any::<OutputDescription>(), 0..10), | |||
vec(any::<u8>(), 64), | |||
) | |||
.prop_map(|(first, rest_spends, rest_outputs, sig)| { | |||
.prop_map(|(first, rest_spends, rest_outputs, sig_bytes)| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -24,6 +24,7 @@ pub struct AddressBook { | |||
span: Span, | |||
} | |||
|
|||
#[allow(clippy::len_without_is_empty)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -115,7 +113,7 @@ impl AddressBook { | |||
pub fn is_potentially_connected(&self, addr: &SocketAddr) -> bool { | |||
let _guard = self.span.enter(); | |||
match self.by_addr.get(addr) { | |||
None => return false, | |||
None => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -32,7 +32,7 @@ pub const HEARTBEAT_INTERVAL: Duration = Duration::from_secs(60); | |||
pub const TIMESTAMP_TRUNCATION_SECONDS: i64 = 30 * 60; | |||
|
|||
/// The User-Agent string provided by the node. | |||
pub const USER_AGENT: &'static str = "🦓Zebra v2.0.0-alpha.0🦓"; | |||
pub const USER_AGENT: &str = "🦓Zebra v2.0.0-alpha.0🦓"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const lifetimes are 'static
by default
@@ -39,7 +39,7 @@ impl Service<Request> for Client { | |||
Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send + 'static>>; | |||
|
|||
fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | |||
if let Err(_) = ready!(self.server_tx.poll_ready(cx)) { | |||
if ready!(self.server_tx.poll_ready(cx)).is_err() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
zebra-network/src/peer_set/set.rs
Outdated
@@ -220,6 +220,7 @@ where | |||
type Future = | |||
Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send + 'static>>; | |||
|
|||
#[allow(clippy::cognitive_complexity)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Okay, sounds like we should try disabling the |
Thanks @hawkw for pointing this out.
Merging this early because it touches a bunch of different places in the source tree so it's difficult to work around. |
overhaul new user experience - you need an invite or you need to pay.
Adds a CI pass to run clippy.
Closes #209.