Skip to content

chore: create and apply rustfmt.toml #213

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

Conversation

luisschwab
Copy link
Member

@luisschwab luisschwab commented Apr 19, 2025

Description

Closes #3.

This PR adds basic formatting configuration for uniform comments. Note that the nightly toolchain is necessary.

Changes:

  • Add rustfmt.toml with this config:
comment_width = 140
format_code_in_doc_comments = true
wrap_comments = true
  • Apply rustfmt.toml.
  • Update the fmt CI job to use the nightly toolchain.
  • Update PR template to use nightly toolchain (cargo fmt -> cargo +nightly fmt).

Note: I had to add #[rustfmt::skip] to test_extract_satisfaction_timelock() because the base64 PSBT would get wrapped due to the slashes.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo +nightly fmt and cargo clippy before committing

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@coveralls
Copy link

coveralls commented Apr 19, 2025

Pull Request Test Coverage Report for Build 14695287352

Details

  • 21 of 21 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 86.537%

Totals Coverage Status
Change from base Build 14270666930: 0.01%
Covered Lines: 7276
Relevant Lines: 8408

💛 - Coveralls

@luisschwab luisschwab changed the title chore: add and apply rustfmt.toml chore: create and apply rustfmt.toml Apr 19, 2025
@notmandatory notmandatory added the chore Non-coding related work label Apr 23, 2025
@notmandatory notmandatory moved this to In Progress in BDK Wallet Apr 23, 2025
@notmandatory notmandatory modified the milestone: 2.0.0 Apr 23, 2025
@notmandatory
Copy link
Member

I didn't realize this would require +nightly. Any idea when the fmt features we need will be in a stable release?

notmandatory
notmandatory previously approved these changes Apr 23, 2025
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 0b5e3f6

It's a little ackward to have to use cargo +nightly fmt but since it only needs to be done before pushing the PR and will make all the docs and docs tests more consistent I'd still like to do it.

@luisschwab
Copy link
Member Author

Any idea when the fmt features we need will be in a stable release?

Not sure, but once it's in stable we can revert it.

By the way, I made an identical PR on bdk as well.

@ValuedMammal
Copy link
Collaborator

Concept ACK

If we do this I think we should remove the extra --config option from the fmt job in CI now that it's in the config file. I guess it technically doesn't need the "prepare" job either because we won't be getting the toolchain from the rust-version file.

For the future I would look at increasing some of the granular width settings such as chain_width to prevent method chains from going vertical too quickly. Can also take some inspiration from rust-bitcoin rustfmt.toml.

I ran cargo +nightly fmt and cargo clippy before committing

You mean I ran just fmt 😄

@luisschwab
Copy link
Member Author

luisschwab commented Apr 26, 2025

If we do this I think we should remove the extra --config option from the fmt job in CI now that it's in the config file.

True.

I guess it technically doesn't need the "prepare" job either because we won't be getting the toolchain from the rust-version file.

Maybe just comment it out so it's easier to revert when these formatting rules become stable?

You mean I ran just fmt 😄

We need one of those!

@ValuedMammal
Copy link
Collaborator

I don't understand what wrap_comments is doing though, is it supposed to also respect the value of comment_width?

@luisschwab
Copy link
Member Author

I got this from the rustfmt docs:

comment_width
Maximum length of comments. No effect unless wrap_comments = true.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK e736cf9

@luisschwab luisschwab moved this from In Progress to Needs Review in BDK Wallet May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Non-coding related work
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

Add project rustfmt.toml file
4 participants