-
Notifications
You must be signed in to change notification settings - Fork 682
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
chore: Apply Clippy lints single_match
and redundant_pattern_matching
#5740
base: develop
Are you sure you want to change the base?
chore: Apply Clippy lints single_match
and redundant_pattern_matching
#5740
Conversation
match self.runtime.sock.take() { | ||
Some(s) => { | ||
let _ = s.shutdown(Shutdown::Both); | ||
} | ||
None => {} | ||
if let Some(s) = self.runtime.sock.take() { | ||
let _ = s.shutdown(Shutdown::Both); | ||
} | ||
|
||
self.runtime.sock = Some(s); |
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.
Maybe makes more sense to rewrite this segment as:
if let Some(mut socket) = self.runtime.sock {
let _ = s.shutdown(Shutdown::Both);
}
Then we don't need the .take()
-> replace()
pattern.
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.
Seems like it makes more sense to use replace()
instead of take()
since we re-assign self.runtime.sock
in the next line anyway
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.
Actually is the call to shutdown()
even necessary? The socket should close when we drop the TcpStream
, right?
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.
Changed to use replace()
in 7e2d60e
…pattern-matching` and fix conflicts
for commit_op in miner.block_commits.iter().rev() { | ||
match SortitionDB::get_block_snapshot_for_winning_stacks_block( | ||
if let Some(sn) = SortitionDB::get_block_snapshot_for_winning_stacks_block( | ||
ic, | ||
&fork_tip.sortition_id, | ||
&commit_op.block_header_hash, | ||
) | ||
.unwrap() | ||
{ | ||
Some(sn) => { | ||
return Some(sn); | ||
} | ||
None => {} | ||
return Some(sn); |
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.
What about find_map
?
miner.block_commits.iter().rev().find_map(|commit_op| {
SortitionDB::get_block_snapshot_for_winning_stacks_block(
ic,
&fork_tip.sortition_id,
&commit_op.block_header_hash,
).unwrap()
})
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.
Yeah that's much better
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.
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.
LGTM, just had a suggestion for a few additional opportunistic refactors.
@@ -264,41 +264,32 @@ impl BitcoinIndexer { | |||
match net::TcpStream::connect((self.config.peer_host.as_str(), self.config.peer_port)) { | |||
Ok(s) => { | |||
// Disable Nagle algorithm | |||
s.set_nodelay(true).map_err(|_e| { | |||
test_debug!("Failed to set TCP_NODELAY: {:?}", &_e); | |||
s.set_nodelay(true).map_err(|e| { |
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.
Looks like test_debug doesn't count towards variable use :/ I find this surprising, but clippy is complaining.
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.
Weird, but I'll undo the change
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.
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.
Looks good. I just triggered a retry of the failing tests.
- tests::nakamoto_integrations::follower_bootup_across_multiple_cycles
- tests::signer::v0::no_reorg_due_to_successive_block_validation_ok
- tests::signer::v0::single_miner_empty_sortition
- net::tests::convergence::test_walk_star_allowed_15
- net::tests::convergence::test_walk_star_15_plain
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.
👍
Description
Apply the following Clippy lints:
single_match
redundant_pattern_matching
These lints reduce certain
match
andif let
statements into simpler forms, and result in 370 fewer lines of code