Skip to content

#3882 post-rustfmt cleanups #3895

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 9 commits into
base: main
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Collaborator

No description provided.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 27, 2025

I've assigned @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Looks more readable to me! Just the (hopefully) two CI fixes and LGTM.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

LGTM after CI is fixed

Having tons of `.node.get_our_nod_id()` spewn across all our tests
doesn't help readability, so here we replace some further cases of
it with local variables in `async_signer_tests.rs`.
Having tons of `.node.get_our_nod_id()` spewn across all our tests
doesn't help readability, so here we replace some further cases of
it with local variables in `functional_test_utils.rs`.
Calling a macro within a method expression is somewhat annoying
to read, and due to `rustfmt` expansion, calling a `handle_*`
method with a `get_event_msg` macro as the second argument tends to
be substantially less readable than just breaking the mess out into
its own variable.
In some cases `rustfmt` makes long boolean expressions fairly
difficult to parse. Here we clean up a specific case in
`functional_test_utils.rs` which was recently exposed.
`rustfmt` loves to make expressions vertical by putting each method
parameter on its own line. In cases where each method parameter is
actually doing something, this can be fine, but in some cases we
have a few method parameters that are straightforward and shouldn't
be the readers focus, mixed with one or two parameters which are
key.

Here, we clean up a regular instance of this in calls to
`check_closed_event` in `shutdown_tests.rs`. It breaks out the
`ClosureReason` (the thing that usually is being tested for) from
the remaining parameters, which are fairly straightforward and not
the important points, leaving the rest of `check_closed_event` on
one line.

This also fixes some instances where `rustfmt` refused to format
code entirely.
`rustfmt` loves to make expressions vertical by putting each method
parameter on its own line. In cases where each method parameter is
actually doing something, this can be fine, but in some cases we
have a few method parameters that are straightforward and shouldn't
be the readers focus, mixed with one or two parameters which are
key.

Here, we clean up a regular instance of this in calls to
`send_payment_with_route` in `priv_short_conf_tests.rs` and
`shutdown_tests.rs`. In these tests, the fact that we're sending a
payment along a route picked on a previous line is important, but
the specific parameters of the payment are not. Thus, condensing
the calls onto a single line by breaking out some parameters into
variables enables the reader to more easily skim past useless
details.
`rustfmt` loves to make expressions vertical by putting each method
parameter on its own line. In cases where each method parameter is
actually doing something, this can be fine, but in some cases we
have a few method parameters that are straightforward and shouldn't
be the readers focus, mixed with one or two parameters which are
key.

Here, we clean up a regular instance of this in calls to
`peer_connected` in `priv_short_conf_tests.rs` and
`shutdown_tests.rs`. In these tests, the fact that we're
reconnecting is important, but the specific features in the `Init`
message are not. Thus, condensing the calls onto two lines by
breaking the init message out into a variable enables the reader
to more easily skim past useless details.
`rustfmt` loves to make expressions vertical by putting each method
parameter or `.` subexpression on its own line. In cases where each
method parameter or `.` subexpression is actually doing something,
this can be fine, but in some cases we have a few method parameters
that are straightforward and shouldn't be the readers focus, mixed
with one or two parameters which are key or a few subexpressions
which simply do rote variable indexing or locking.

Here we clean up various `rustfmt` verticality that exposes
unimportant details of tests and test utils, making it easier for
the reader to skim past them by introducing additional variables.
The last few commits made various cleanups to some tests files,
which we re-`rustfmt` here.
@TheBlueMatt TheBlueMatt force-pushed the 2025-06-3882-followups branch from 4725228 to 4aa6d39 Compare June 27, 2025 20:54
@TheBlueMatt
Copy link
Collaborator Author

Ha, sorry about that.

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.

4 participants