Rustfmt chainmonitor.rs#3847
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
Presumably we should just format all of |
|
Happy to do that. I just wanted to be conservative and only format the parts that I touch. |
joostjager
force-pushed
the
fmtskip-chainmonitor
branch
from
df28592 to
d0e501a
Compare
|
Pushed full rustfmt'ed chainmonitor |
joostjager
changed the title
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
I'm fine to do these in follow-up if you want to Just Land
lightning/src/chain/chainmonitor.rs
Outdated
| self.monitors | ||
| .read() | ||
| .unwrap() | ||
| .iter() |
There was a problem hiding this comment.
nit: extract into var
There was a problem hiding this comment.
Based on CI failures, looks like you might have to extract only self.monitors.read().unwrap()
| match super::channelmonitor::process_events_body!( | ||
| self.monitors.read().unwrap().get(&channel_id).map(|m| &m.monitor), self.logger, ev, handler(ev).await) { | ||
| self.monitors.read().unwrap().get(&channel_id).map(|m| &m.monitor), | ||
| self.logger, | ||
| ev, | ||
| handler(ev).await | ||
| ) { |
There was a problem hiding this comment.
I'd prefer:
for channel_id in mons_to_process {
let mut ev;
let monitor = self.monitors.read().unwrap().get(&channel_id).map(|m| &m.monitor);
let res = process_events_body!(monitor, self.logger, ev, handler(ev).await);
match res {
Ok(()) => {},
Err(ReplayEvent()) => {
self.event_notifier.notify();
},
}
}with importing process_events_body
There was a problem hiding this comment.
This starts to complain about borrow from a temporary value. Left as is, this is formatting only?
|
Certainly if you don't want to bother cleaning things up we can just land a subset that doesn't need it and come back to it later, I was just suggesting it probably wasn't much effort to clean up as we went given the file's size. |
|
Right. Yes, the clean up is kind of subjective, so wanted to avoid that. But will now go with @valentinewallace's suggestions above. |
joostjager
force-pushed
the
fmtskip-chainmonitor
branch
from
ab6d893 to
449591a
Compare
|
Rebased |
joostjager
force-pushed
the
fmtskip-chainmonitor
branch
from
449591a to
3242b93
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Gonna go ahead and land but will clean things up a bit more in a followup.
| #[cfg(not(c_bindings))] | ||
| assert!(nodes[1].chain_monitor.chain_monitor.list_pending_monitor_updates().get(&channel_id) | ||
| .unwrap().contains(&next_update)); | ||
| assert!(nodes[1] |
There was a problem hiding this comment.
Probably
worth
making
this
less
vertical
:)
| check_closed_event!(&nodes[2], 1, ClosureReason::CommitmentTxConfirmed, false, | ||
| [nodes[0].node.get_our_node_id()], 1000000); | ||
| let closure_reason = ClosureReason::CommitmentTxConfirmed; | ||
| let node_a_id = nodes[0].node.get_our_node_id(); |
There was a problem hiding this comment.
Probably worth making this change across the two functional tests, given it somewhat reduces cognitive load when reading tests.
4 participants
I badly miss rustfmt for my changes in #3778.
The larger strategy for rustfmt is still being discussed (#3749, #3809), but getting this one with immediate benefit through would be great.