Introduce rustfmt in a non-invasive manner #1040

tcharding · 2022-06-06T04:42:43Z

Looks like we are getting to a place that rustfmt might be able to be used. In an effort to introduce rustfmt without requiring devs to do excessively mundane reviews introduce rustfmt in a non-invasive manner. What this means is

Add a fully fleshed out rustfmt config file that explicitly ignores all the source files
Enable sections of code one by one so review is easier (including preparatory patches before doing each section).

This PR currently does examples and all the source files at the root level of the crate (i.e. excludes all directories in src/). The other directories can then be done one at a time.

Please see discussion on: #959 for more context.

dpc · 2022-06-06T05:10:29Z

Might have been easier to start without 0f779e1, but the more the merrier if you ask me. :D

apoelstra

ACK 0f779e1

tcharding · 2022-06-07T01:48:19Z

Might have been easier to start without 0f779e1, but the more the merrier if you ask me. :D

Thanks for the review! I can remove that patch if anyone else finds this too arduous to review. I was trying to walk the line between getting something done and being a real pain to review :)

tcharding · 2022-06-07T01:51:29Z

Changes in force push: Just rebase on master fixing the merge conflicts in a call to hash_newtype.

apoelstra

ACK 7b2f48b

tcharding · 2022-06-20T04:44:56Z

Any more love for this one crew?

Kixunil

TBH, I'm still unconvinced. :( Wouldn't block if most people want this, just please at least consider practical implications of method call chains being on a single line and import reordering.

Kixunil · 2022-06-20T11:44:14Z

rustfmt.toml

+imports_granularity = "Module"     # Default "Preserve"
+group_imports = "StdExternalCrate" # Default "Preserve"
+reorder_imports = true
+reorder_modules = true


I really don't like these much. Sometimes it may be nicer to logically group the imports, e.g. pub use at top. Sadly, there's no way to implement such logic in rustfmt.

IMO "reliable benefit of predictable ordering" > "barely ever used benefit of expressing some logical groups, that also has to be maintained over time (which it won't)".

+1 for what @dpc said. I do understand the desire to have 'pub use' statements separate but its just another implicit policy for devs to have to remember and like dpc said such groupings never get maintained.

TBH I'm kind of disheartened by this discussion, taking care of the import statements is, IMO, a no-brainer. If we cannot come to some basic agreement that at some stage its better to use tools than developer time it is really hard to make progress on this PR.

Lets not become a project where no changes can be made because there is always some negative in any change - implying that we get stuck with negatives we are unable to address. rustfmt is not perfect, nothing is, rustfmt is, an can always be, constantly improved but refusing to use the tools we have just stagnates us.

Honestly, I don't care about order being lexicographically sorted much and if there's no setting to put pub items first (what I do in my projects) and enforcing is deemed to be too annoying, I don't care about these flags much. If there was a setting I'd want it on and don't care about the rest.

Oh, I remembered one more annoying thing about sorting: if you rename an item then you also have to move it so instead of clear -+ diffl lines being one after another you get them separated. Not saying that it's worth turning off sorting but if there was a nicer solution, that'd be great.

Agree on the diff lines separated thing. So just to make sure I've got it, we can leave the two 'reorder' flags on and acknowledge that it is a desired improvement to rustfmt to be able to separate 'pub use' statements from other imports?

Yep, I was thinking about opening issues at their repo once we agree what we need.

Kixunil · 2022-06-20T11:47:33Z

examples/handshake.rs

-        .duration_since(UNIX_EPOCH)
-        .expect("Time error")
-        .as_secs();
+    let timestamp = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time error").as_secs();


I prefer these to be always on a new line. The practical consequence is that you can quickly comment out things if types match and diffs are easier to understand. I had real-life experience with long iterator chain where types were preserved (lot of maps) and could easily experiment with the code by commenting/adding lines.

Bleh, there's another practical consequence of such things - people miss context when reviewing/reading code
Cause
Things
End
Up
Too
Vertical.

IMO: If a chain of things is so long that it hinders your ability to see stuff, you have some other problems to address.

Like in this case the whole thing could be some fn get_current_unix_ts_secs(), because why would anyone wanted to parse this lower-level 4 element implementation detail, when looking at the higher level code.

It's an example code, so 🤷 , but this whole function could be more idiomatically:

fn build_version_message(address: SocketAddr) -> message::NetworkMessage { message::NetworkMessage::Version( VersionMessage::new() .service_flags(ServiceFlags::NONE), .timestamp(get_current_unix_ts_secs() as i64), .recv_addr(Address::new(&address, ServiceFlags::NONE)), .from_addr(Address::new(&my_address, ServiceFlags::NONE)), /* ... */ .build() ) }

with comments about what each thing means on the builder methoder, instead of each variable, and 1/4 of the LoC size, while being much easier to understand.

BTW. I wish Rust just made a typed builder a language thing, because in the absence of optional and named arguments, builders are a common necessity.

IMO chain_width should really be 100, I thought it was a concrete line length not % lol.

I really, really, really dont understand the rust fascination with deliberately ensuring code is as
vertical
as
is
at
all
possible
to
make
sure
you
lose
all
context.
If we say we want lines up to 100 chars long, that should include chains, I'm not sure why chains are in any way special?

ok, so that is a vote for

max_width = 100 use_small_heuristics = "Max"

And remove all the granular width configuration options, I don't believe we had previously considered that.

Meh, neither is great and if there even exists optimal line, I don't think it can be expressed in code. So I give up on this discussion, do as you wish.

EDIT: Deleted my comment after seeing more discussion on this below.

src/hash_types.rs

src/internal_macros.rs

tcharding · 2022-06-21T00:31:53Z

Thanks for the review and your opinions @Kixunil, this is a hard PR to get consensus on but I'm going to keep trying :)

tcharding · 2022-06-21T00:33:15Z

Changes in force push:

rebased on master
added 3 preparatory patches before running formatter on src/

tcharding · 2022-06-21T00:37:13Z

In rust-bitcoin/rust-miniscript#434 (comment) @sanket1729 aired a view that using imports_granularity = "Module" leads to increased merge conflicts and in rust-bticoin because we have the '2 explicit acks before merge' policy this leads to devs having to re-ack PRs when the only change was an import statement merge conflict. Perhaps we should use import_granularity = "Item" to counter-act this. I would add to this that we should, IMO, use the same rustfmt config file across the whole rust-bitcoin organization though.

Kixunil · 2022-06-21T10:04:16Z

Good point about merges but merging can lead to other issues - duplicated imports or conflicts on consecutive lines. IMO the best solution would be semantic merging but he only solution I know of doesn't support Rust, is proprietary and from my experience the UI is buggy at least on non-Windows platforms. It's free for FOSS and I wouldn't even mind paying if it had Rust support and higher reliability. Being proprietary doesn't bother me for Open Source code.

apoelstra · 2022-06-21T12:40:19Z

I think that, even if we had a smarter merge tool, we'd still be unhappy with Github saying (falsely) that there were merge conflicts, since we can't control the algorithm they use for conflictt detection.

Having said this, I honestly don't mind re-acking stuff where they were rebased and import stuff resolved ... I guess it's annoying for the person doing the rebase, and causes delays, but as a reviewer it's a very mechanical thing to run range-diff, see that the only conflicting lines are use statements, then restart my CI script and do something else until I get the alert that it's done.

tcharding · 2022-06-21T23:24:40Z

Thanks crew, this PR was pissing me off yesterday and today reading the responses I got two chuckles out of it - so top effort for bringing humour in arduous work. We are making progress :)

Changes in force push:

Change chain_width to be the default (60) instead of the custom 80
The chain_width change effected the examples patch but did not effect the src/ patch.

TheBlueMatt · 2022-06-22T17:57:24Z

Change chain_width to be the default (60) instead of the custom 80

Sorry just got to responding to the comments from yesterday. I'd actually really strongly prefer this be 100%. I'm really not at all sure why chains are somehow "special" compared to other code. If we're okay reading lines that are 100 chars long, I don't really get why we're not okay reading chains that are 100 chars long? Both are just as much mental overhead to read, and both have the same context-scanability tradeoffs.

dpc · 2022-06-22T18:12:39Z

Seems like Matt is feeling most strongly about it and personally I would be happy with 100%. Doesn't bother me at all, as long as rustfmt takes care of it. I guess we can optimize for people hating it least. 😆

Kixunil · 2022-06-23T06:33:51Z

Just realized if we had all code rustfmted people could just set their own settings during development and reformat using official one before committing.

apoelstra · 2022-06-23T14:08:12Z

@Kixunil well, it wouldn't help people trying to read diffs which are extra-dense due to people cramming tons of crap into one line ... but agreed with "don't care anymore, let's just do something".

tcharding · 2022-06-23T23:26:57Z

... but agreed with "don't care anymore, let's just do something".

I think we are all getting to the same state of mind, lets push this through.

tcharding · 2022-06-23T23:28:09Z

I've set use_small_heuristics = "Max", commented out all the granular config options, and rebased.

tcharding · 2022-06-24T00:01:43Z

Needs #1063 to get green CI run.

Kixunil · 2022-06-24T14:08:47Z

@apoelstra it'd be quite easy to create a script that formats both commits and runs diff. Might be less convenient but at least it's something.

apoelstra · 2022-06-24T16:14:28Z

That's a neat idea, I'll try that sometime.

tcharding · 2022-07-14T22:51:48Z

@Kixunil are you ok with this or are you still wanting to explore the scripting idea?

@TheBlueMatt would you be willing to sign off on this, at a minimum to show others that your views, as much as possible, have been taken into consideration?

Kixunil

ACK 1a8d127

Looks like lesser evil.

tcharding · 2022-07-19T01:17:16Z

Sorry folks had to rebase to fix merge conflicts.

tcharding · 2022-07-19T01:45:29Z

Don't ack, needs another rebase :)

Add a configuration file but explicitly ignore all source files. The aim of this is that we can then un-ignore files a few at a time and deal with issues in a small isolated manner.

Remove "examples" from the rustfmt ignore list.

In preparation for enabling rustfmt on `lib.rs` refactor the `compile_error` call so that the formatter does not touch it.

In preparation for running the formatter on root level modules and a submodule that holds all the macro calls in `hash_types` so we can configure rustfmt to skip formatting them.

In preparation for running the formatter on all source files at the root of the crate; skip formatting `prelude` module because we use unconventional import statements so they to save writing a million compiler attributes.

The local variable `formatter` can be shortened to `f` with no loss of clarity since it is so common. Done in preparation for running `rustfmt` on `src`.

In preparation for running the formatter on `src/` and a function local use statement for `$crate::serde::de::Unexpected`, this shortens the line of code that uses this type preventing the formatter for later munging that line.

Remove the ignore for the "src" directory, this enables the formatter for all source files in the `src/` directory.

tcharding · 2022-07-19T03:44:22Z

Rebased on master, no other changes.

junderw · 2022-07-19T07:06:30Z

Maybe we should pause merging all other PRs before merging this...?

Linter/Formatter PRs are rebase magnets.

apoelstra

ACK ee9a3ec

sanket1729

ACK ee9a3ec

sanket1729 · 2022-07-19T20:37:01Z

Reviewed the minor range-diff from @Kixunil ACK on 1a8d127 and ee9a3ec. Considering that as the third ACK for this and merging this.

apoelstra · 2022-07-19T21:08:32Z

Maybe we should pause merging all other PRs before merging this...?

Linter/Formatter PRs are rebase magnets.

I think it's alright :) people have been pretty active and ready to rebase lately.

tcharding · 2022-07-19T22:00:57Z

Can't believe this merged, rust-bitcoin has officially been dragged, kicking and screaming, into the 21st century :)

…sive manner ee9a3ec Enable formatter for "src" (Tobin C. Harding) 743b197 Add use for Unexpected (Tobin C. Harding) fd217e7 Use f instead of formatter (Tobin C. Harding) 7b50a96 Do not format prelude module (Tobin C. Harding) 01471f7 Shield hash_newtypes from rustfmt (Tobin C. Harding) 65d19d9 Refactor compile_error message (Tobin C. Harding) 6461e2d Run formatter on examples/ (Tobin C. Harding) fd1c658 Add a rustfmt configuration file with explicit ignores (Tobin C. Harding) Pull request description: Looks like we are getting to a place that rustfmt _might_ be able to be used. In an effort to introduce `rustfmt` without requiring devs to do excessively mundane reviews introduce `rustfmt` in a non-invasive manner. What this means is - Add a fully fleshed out `rustfmt` config file that explicitly ignores all the source files - Enable sections of code one by one so review is easier (including preparatory patches before doing each section). This PR currently does `examples` and all the source files at the root level of the crate (i.e. excludes all directories in `src/`). The other directories can then be done one at a time. Please see discussion on: rust-bitcoin/rust-bitcoin#959 for more context. ACKs for top commit: sanket1729: ACK ee9a3ec apoelstra: ACK ee9a3ec Tree-SHA512: f4ff3c031b5d685777e09bac2df59ed8217576b807da7699f44fe00aa509d0b7fe47617a8b3eff00d3125aeece3e010b8f9dd8733d315ccb2adc0e9a7d7f07e3

tcharding mentioned this pull request Jun 6, 2022

Enable rustfmt #959

Closed

apoelstra previously approved these changes Jun 6, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 7b2f48b June 7, 2022 01:51

tcharding force-pushed the 06-06-rustfmt-by-file branch from 0f779e1 to 7b2f48b Compare June 7, 2022 01:51

apoelstra previously approved these changes Jun 7, 2022

View reviewed changes

Kixunil reviewed Jun 20, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via ad71ae8 June 21, 2022 00:32

tcharding force-pushed the 06-06-rustfmt-by-file branch from 7b2f48b to ad71ae8 Compare June 21, 2022 00:32

tcharding force-pushed the 06-06-rustfmt-by-file branch from ad71ae8 to e53e302 Compare June 21, 2022 23:15

tcharding force-pushed the 06-06-rustfmt-by-file branch from e53e302 to b45060f Compare June 23, 2022 23:25

tcharding mentioned this pull request Jun 24, 2022

Tracking issue for rustfmt #788

Closed

Kixunil previously approved these changes Jul 15, 2022

View reviewed changes

tcharding dismissed stale reviews from Kixunil, junderw, apoelstra, and dpc via 3099023 July 19, 2022 01:17

tcharding force-pushed the 06-06-rustfmt-by-file branch from 1a8d127 to 3099023 Compare July 19, 2022 01:17

tcharding added 8 commits July 19, 2022 13:43

Add a rustfmt configuration file with explicit ignores

fd1c658

Add a configuration file but explicitly ignore all source files. The aim of this is that we can then un-ignore files a few at a time and deal with issues in a small isolated manner.

Run formatter on examples/

6461e2d

Remove "examples" from the rustfmt ignore list.

Refactor compile_error message

65d19d9

In preparation for enabling rustfmt on `lib.rs` refactor the `compile_error` call so that the formatter does not touch it.

Shield hash_newtypes from rustfmt

01471f7

In preparation for running the formatter on root level modules and a submodule that holds all the macro calls in `hash_types` so we can configure rustfmt to skip formatting them.

Do not format prelude module

7b50a96

In preparation for running the formatter on all source files at the root of the crate; skip formatting `prelude` module because we use unconventional import statements so they to save writing a million compiler attributes.

Use f instead of formatter

fd217e7

The local variable `formatter` can be shortened to `f` with no loss of clarity since it is so common. Done in preparation for running `rustfmt` on `src`.

Add use for Unexpected

743b197

In preparation for running the formatter on `src/` and a function local use statement for `$crate::serde::de::Unexpected`, this shortens the line of code that uses this type preventing the formatter for later munging that line.

Enable formatter for "src"

ee9a3ec

Remove the ignore for the "src" directory, this enables the formatter for all source files in the `src/` directory.

tcharding force-pushed the 06-06-rustfmt-by-file branch from 3099023 to ee9a3ec Compare July 19, 2022 03:43

0xB10C mentioned this pull request Jul 19, 2022

Add BIP152 (Compact Blocks) structures #1088

Merged

apoelstra approved these changes Jul 19, 2022

View reviewed changes

sanket1729 approved these changes Jul 19, 2022

View reviewed changes

sanket1729 merged commit e5acc07 into rust-bitcoin:master Jul 19, 2022

tcharding deleted the 06-06-rustfmt-by-file branch July 20, 2022 01:37

DanGould mentioned this pull request Aug 12, 2022

Clean up argument parsing. payjoin/nolooking#5

Merged

Introduce rustfmt in a non-invasive manner #1040

Introduce rustfmt in a non-invasive manner #1040

Conversation

tcharding commented Jun 6, 2022 • edited

dpc commented Jun 6, 2022 • edited by tcharding

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jun 7, 2022

tcharding commented Jun 7, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jun 20, 2022 • edited

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc Jun 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Jun 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Jun 21, 2022

tcharding commented Jun 21, 2022

tcharding commented Jun 21, 2022 • edited

Kixunil commented Jun 21, 2022

apoelstra commented Jun 21, 2022 • edited

tcharding commented Jun 21, 2022

TheBlueMatt commented Jun 22, 2022

dpc commented Jun 22, 2022

Kixunil commented Jun 23, 2022

apoelstra commented Jun 23, 2022

tcharding commented Jun 23, 2022 • edited

tcharding commented Jun 23, 2022

tcharding commented Jun 24, 2022

Kixunil commented Jun 24, 2022

apoelstra commented Jun 24, 2022

tcharding commented Jul 14, 2022

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented Jul 19, 2022

tcharding commented Jul 19, 2022

tcharding commented Jul 19, 2022

junderw commented Jul 19, 2022

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 commented Jul 19, 2022

apoelstra commented Jul 19, 2022

tcharding commented Jul 19, 2022

tcharding commented Jun 6, 2022 •

edited

dpc commented Jun 6, 2022 •

edited by tcharding

tcharding commented Jun 20, 2022 •

edited

dpc Jun 20, 2022 •

edited

TheBlueMatt Jun 22, 2022 •

edited

tcharding commented Jun 21, 2022 •

edited

apoelstra commented Jun 21, 2022 •

edited

tcharding commented Jun 23, 2022 •

edited