Initiate configuration of rustfmt #374

tcharding · 2022-04-28T01:21:26Z

We currently use rustfmt with the default configuration. There is some interest in using rustfmt in the crates lower down the stack but it is contentious and difficult to find a configuration, if one exists, that is 'good' for everyone. By configuring rustfmt in this crate we can reduce the scope of the discussion from 'should we even do this' down to 'this is a good formatting configuration', with the hope that if we can get a good configuration worked out in this crate it will help ease the debate in the others.

To get this started add a default config file and then set two, hopefully non-contentious, config options.

Please note carefully; the first patch changes CI to run linting checks using the nightly toolchain.

Re: edition 2018 changes, there are some changes to imports in the examples/ that appear in this PR that should really be removed altogether, leaving as is so as not to clutter this PR.

sanket1729

concept ACK. We can merge this right before 2018 bump

.github/workflows/rust.yml

tcharding · 2022-04-28T23:54:49Z

Changes in force push:

Added initial patch to the PR that does the lint->fmt change.
Changes to patch 2 as required be newly added patch 1.
No other changes.

tcharding · 2022-04-29T02:25:01Z

Will require manual configuration of the CI pipeline in order to merge.

tcharding · 2022-05-02T02:45:09Z

Rebased.

apoelstra

ACK 9c053e5

Nice!

tcharding · 2022-05-10T04:33:44Z

Rebased and re-did the second two patches i.e., for each, updated the config option, ran cargo +nightly fmt and used the same commit message.

apoelstra

ACK 8e02ed0

sanket1729 · 2022-05-10T19:38:52Z

There are merge conflicts here. This would need a rebase again :(

We only run the formatter (`cargo fmt`) not the linter (`cargo clippy`). Use the appropriate term in the test script and the CI configuration.

tcharding · 2022-05-10T21:27:45Z

Changes in force push:

rebased on master, CI workflow had conflicts
did formatting in last two patches as before

sanket1729 · 2022-05-10T22:00:53Z

Will require manual configuration of the CI pipeline in order to merge.

Can you elaborate? Do you mean this would need to be merged in god mode?

tcharding · 2022-05-10T22:30:57Z

Will require manual configuration of the CI pipeline in order to merge.

Can you elaborate? Do you mean this would need to be merged in god mode?

I don't know what setting has to be changed, I don't have access to settings so I'm going off what was said on another PR/repo when I changed the CI workflow. When a PR changes a workflow job the old one seems to still appear in the checks list on the web interface and the new one does not. Sorry I'm not very knowledgeable with GitHub.

sanket1729 · 2022-05-10T22:40:06Z

I think there is a bug in current config? See https://github.com/rust-bitcoin/rust-miniscript/actions/runs/2303431754

I don't have access to settings so I'm going off what was said on another PR/repo when I changed the CI workflow

I can set the changes, but I cannot see the options for the fuzz-names in the CI workflow for this PR. Perhaps it is related to the above issue?

Running `rustfmt` with the nightly toolchain allows us to use unstable options. Users can still run it with the stable toolchain locally and ignore the warnings.

Generate and add a default configuration file for `rustfmt` using rustfmt --print-config default rustfmt.toml This produces a config file that causes `rustfmt` to make no changes, as expected, when we run cargo +nightly fmt Please note, the config file includes unstable options which throw warnings if used without the nightly toolchain.

Configure `group_imports = "StdExternalCrate"`. The benefit of this option is that it increases uniformity in the code base over the default "Preserve", while saving devs needing to think about where they place their import statements (for those that do not use tooling to add them).

Configure `imports_granularity = "Module"`. The benefit of this option is that it increases uniformity in the code base over the default "Preserve" and also, from my personal experience, it tends to reduce the noise in diffs from adding/removing imports.

tcharding · 2022-05-10T22:45:53Z

Well done, you were correct. Was missing a run: ./contrib/test.sh line. Thanks

sanket1729

reACK 8062016

sanket1729 · 2022-05-10T23:13:58Z

Verified the diff betwen 4847894 and 8062016.

…tfmt 80620166e6372b0801850edcd6c103718467ebfe Configure imports_granularity (Tobin C. Harding) a89edba4e79061e5194b6ca80a3388d18496deee Configure group_imports (Tobin C. Harding) ccdef6facdf13adbb611b5ccda1d5bed6ebc2f92 Add rustfmt configuration file (Tobin C. Harding) f7f6f3bde6a7a9cdedb7bc16ef0d510e3d8af54f Run cargo fmt with nightly toolchain (Tobin C. Harding) 8d0c69a27dc84cc4fafe5f840e26e3bb8f257500 Use term fmt instead of lint (Tobin C. Harding) Pull request description: We currently use `rustfmt` with the default configuration. There is some interest in using `rustfmt` in the crates lower down the stack but it is contentious and difficult to find a configuration, if one exists, that is 'good' for everyone. By configuring `rustfmt` in this crate we can reduce the scope of the discussion from 'should we even do this' down to 'this is a good formatting configuration', with the hope that if we can get a good configuration worked out in this crate it will help ease the debate in the others. To get this started add a default config file and then set two, hopefully non-contentious, config options. Please note carefully; the first patch changes CI to run linting checks using the nightly toolchain. Re: edition 2018 changes, there are some changes to imports in the `examples/` that appear in this PR that should really be removed altogether, leaving as is so as not to clutter this PR. ACKs for top commit: sanket1729: reACK 80620166e6372b0801850edcd6c103718467ebfe Tree-SHA512: 2b8967752fa659eb6a66dfd6a4ea817547c0212e740624011cdc6a272f9f5aa4e28fa9f4b3726ac535b61b64441c4b475125b0445ea207631a1792c3d6823561

tcharding force-pushed the rustfmt branch 3 times, most recently from 16a867d to dfc3e76 Compare April 28, 2022 02:41

sanket1729 reviewed Apr 28, 2022

View reviewed changes

.github/workflows/rust.yml Outdated Show resolved Hide resolved

tcharding force-pushed the rustfmt branch from dfc3e76 to 60a5751 Compare April 28, 2022 23:54

tcharding force-pushed the rustfmt branch 3 times, most recently from ebcd453 to 49a08bd Compare May 3, 2022 00:18

tcharding mentioned this pull request May 3, 2022

Re-work the CI pipeline and test script #370

Closed

tcharding force-pushed the rustfmt branch from 49a08bd to 9c053e5 Compare May 3, 2022 00:40

apoelstra previously approved these changes May 3, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 2034c17 May 10, 2022 04:27

tcharding force-pushed the rustfmt branch 2 times, most recently from 2034c17 to 8e02ed0 Compare May 10, 2022 04:32

apoelstra previously approved these changes May 10, 2022

View reviewed changes

Use term fmt instead of lint

8d0c69a

We only run the formatter (`cargo fmt`) not the linter (`cargo clippy`). Use the appropriate term in the test script and the CI configuration.

tcharding dismissed apoelstra’s stale review via 4847894 May 10, 2022 21:26

tcharding force-pushed the rustfmt branch from 8e02ed0 to 4847894 Compare May 10, 2022 21:26

tcharding added 3 commits May 11, 2022 08:45

Run cargo fmt with nightly toolchain

f7f6f3b

Running `rustfmt` with the nightly toolchain allows us to use unstable options. Users can still run it with the stable toolchain locally and ignore the warnings.

Configure imports_granularity

8062016

Configure `imports_granularity = "Module"`. The benefit of this option is that it increases uniformity in the code base over the default "Preserve" and also, from my personal experience, it tends to reduce the noise in diffs from adding/removing imports.

tcharding force-pushed the rustfmt branch from 4847894 to 8062016 Compare May 10, 2022 22:45

tcharding mentioned this pull request May 10, 2022

Tracking issue for rustfmt rust-bitcoin/rust-bitcoin#788

Closed

sanket1729 approved these changes May 10, 2022

View reviewed changes

sanket1729 merged commit 5c466a8 into rust-bitcoin:master May 10, 2022

tcharding deleted the rustfmt branch May 11, 2022 02:16

tcharding mentioned this pull request May 26, 2022

Enable rustfmt rust-bitcoin/rust-bitcoin#959

Closed

Initiate configuration of rustfmt #374

Initiate configuration of rustfmt #374

Uh oh!

Conversation

tcharding commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Apr 29, 2022

Uh oh!

tcharding commented May 2, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented May 10, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented May 10, 2022

Uh oh!

tcharding commented May 10, 2022

Uh oh!

sanket1729 commented May 10, 2022

Uh oh!

tcharding commented May 10, 2022

Uh oh!

sanket1729 commented May 10, 2022

Uh oh!

tcharding commented May 10, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented May 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tcharding commented Apr 28, 2022 •

edited

Loading

tcharding commented Apr 28, 2022 •

edited

Loading