Skip to content
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

Introduce rustfmt #499

Merged
merged 3 commits into from Nov 17, 2022
Merged

Introduce rustfmt #499

merged 3 commits into from Nov 17, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Nov 2, 2022

(Includes the patch from #504, I pulled it out of this to merge faster)

Introduce rustfmt by doing:

  • Copy the rustfmt config file from rust-bitcoin
  • Prepare the codebase by adding #[rustfmt::skip] as needed and doing some manual format improvements.
  • Run the formatter: cargo +nightly fmt
  • Add formatting checks to CI and the pre-commit hook

Thanks in advance for doing the painful review on patch 3.

@tcharding
Copy link
Member Author

WTF, the format change fail in CI is to a line that I cannot find in locally.

@dpc
Copy link
Contributor

dpc commented Nov 3, 2022

(A bit off-topic) @tcharding What do you think about: rust-lang/rustfmt#5584 ?

@tcharding
Copy link
Member Author

(A bit off-topic) @tcharding What do you think about: rust-lang/rustfmt#5584 ?

I like the idea, not sure why the lad is so unequivocally against it. I don't think it helps us though since "existing" is unformatted the "desired" would never be triggered.

@dpc
Copy link
Contributor

dpc commented Nov 3, 2022

I don't think it helps us though since "existing" is unformatted the "desired" would never be triggered.

That's a good point. It only helps projects that are already formatted.

As we did in `rust-bitcoin` introduced a `rustfmt` configuration file
that is palatable to devs.

Do not run formatter, that is done as a separate patch to assist review.
In preparation for running the formatter do minor refactorings and add a
bunch of `rustfmt::skip` directives.
Run the command `cargo +nightly fmt` to fix formatting issues.

The formatter got confused in one place, adding an incorrect
indentation, this was manually fixed.
@tcharding
Copy link
Member Author

Please note the recently added deprecated "since" strings are so long that they now go over multiple lines, its ugly but I figured since we remove them in a release anyway I'd leave them as is. Can change if required.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK e0e575d

@apoelstra
Copy link
Member

Will hold off on merging this for a bit since there are other PRs in flight and I think this'll conflict with everything.

@tcharding
Copy link
Member Author

For me its better to just merge this one first please, I find these rustfmt PRs super boring to do where as I'm ok to rebase the others because they are much more interesting work. The only active PRs are my two I think.

@apoelstra
Copy link
Member

Hah, fair enough. Merging now.

@apoelstra apoelstra merged commit a777942 into rust-bitcoin:master Nov 17, 2022
@tcharding tcharding deleted the 11-02-rustfmt branch November 17, 2022 20:40
@apoelstra apoelstra mentioned this pull request Nov 19, 2022
apoelstra added a commit that referenced this pull request Nov 22, 2022
75f3886 Add cargo fmt to pre-commit githook (Tobin C. Harding)
0516dde Add formatting check to CI (Tobin C. Harding)
c7807df Run the formatter (Tobin C. Harding)

Pull request description:

  We recently introduced `rustfmt` to the codebase but I forgot to turn it on in CI.

  - Patch 1: Preparatory formatting fixes, introduced since we merged the [formatting PR](#499)
  - Patch 2: Enable formatting in CI
  - Patch 3: Add formatting to the pre-commit hook

ACKs for top commit:
  apoelstra:
    ACK 75f3886

Tree-SHA512: 5ac4ab4015a9728ef890e0c4fe90afcb5e45ab7665da5a8ee289dc877c1ea5c6236e54b68b7122841597864b04606c8bfae7dec86c4b6be74d32437299057b5f
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.

None yet

3 participants