Add a `rustfmt.toml` #8982

expenses · 2021-06-01T14:10:28Z

See #8981. My idea for this PR is to attempt to come up with some rustfmt.toml settings that fit the current style of the codebase as much as possible. I've only ran it the stuff in utils so far as there are less files here than elsewhere.

TriplEight · 2021-06-01T15:03:19Z

https://github.com/reviewdog/reviewdog might be of a help here

expenses · 2021-06-03T12:11:01Z

Needs to be run with cargo +nightly fmt as some settings are currently only available on nightly.

utils/fork-tree/src/lib.rs

expenses · 2021-06-03T12:26:08Z

I think this is the closest style to ours that we can currently get with cargo fmt. I'm happy to either merge or close this PR depending on whether this is acceptable.

rustfmt.toml

utils/build-script-utils/src/version.rs

utils/frame/rpc/support/src/lib.rs

burdges · 2021-06-03T15:16:40Z

utils/browser/src/lib.rs

@@ -151,12 +152,11 @@ pub fn start_client(mut task_manager: TaskManager, rpc_handlers: RpcHandlers) ->
 			Box::pin(async move {
 				let _ = task_manager.future().await;
 			}),
-		).map(drop)
+		)
+		.map(drop),


Just curious, can this newline be disabled? I donno substrate's exiting style though.

overflow_delimited_expr can help sometimes in these situations but on balence I think the code works better left to the default value.

I think the original is indeed better readable here.
Ideally would be a rule to:

keep (..).map(drop), as such

while letting (..).map(drop).foo(bar) gets split eventually, whether always or dependending on the line width (I would be fine with both options eitherway)

utils/browser/src/lib.rs

burdges · 2021-06-03T21:47:48Z

It maybe does not come up in substrate much, but how do you preserve rustfmt mangling a significant mixture of new lines and one liners when running rust fmt?

It helps distinguish mathematical stuff going on in long iterator chains ala https://github.com/w3f/schnorrkel/blob/master/src/vrf.rs#L967-L969 It'd be doable by using named temporaries, except then closures break NLL.

expenses · 2021-06-04T00:46:12Z

It maybe does not come up in substrate much, but how do you preserve rustfmt mangling a significant mixture of new lines and one liners when running rust fmt?

I don't think it's a problem to use #[rustfmt::skip] on things like that, provided it's not used so much that it makes everything ugly.

utils/build-script-utils/src/git.rs

utils/build-script-utils/src/lib.rs

drahnr · 2021-06-07T07:03:50Z

utils/frame/benchmarking-cli/src/writer.rs

-		let first_benchmark = &mapped_results.get(
-			&("first_pallet".to_string(), "instance".to_string())
-		).unwrap()[0];
+		let mapped_results = map_results(


Is there a way to tune the insertion of \n between ( and &[. A few pages down the reverse change is introduced.

This might be due to inconsistent formatting in the original code, but I'm not sure.

I don't think that the formatting of the original code matters much to rustfmt. I suspect this is just an artifact of its line-packing heuristics acting up.

overflow_delimited_expr = true

might improve the situation though.

I didn't see an improvement when I tried this.

kianenigma · 2021-07-19T12:06:59Z

I highly recommend

comment_width = 100
wrap_comments = true
format_strings = true

as well to clean up all of our docstring (which is totally arbitrary-length atm).

expenses · 2021-07-19T12:53:57Z

I highly recommend
comment_width = 100
wrap_comments = true
format_strings = true
as well to clean up all of our docstring (which is totally arbitrary-length atm).

I'd like to have these as well, but they occasionally fail to format strings correctly:

diff --git a/utils/wasm-builder/src/builder.rs b/utils/wasm-builder/src/builder.rs
index 20f33583b8..1d6f936fe3 100644
--- a/utils/wasm-builder/src/builder.rs
+++ b/utils/wasm-builder/src/builder.rs
@@ -195,8 +195,8 @@ fn provide_dummy_wasm_binary_if_not_exist(file_path: &Path) {
 	if !file_path.exists() {
 		crate::write_file_if_changed(
 			file_path,
-			"pub const WASM_BINARY: Option<&[u8]> = None;\
-			 pub const WASM_BINARY_BLOATY: Option<&[u8]> = None;",
+			"pub const WASM_BINARY: Option<&[u8]> = None;pub const WASM_BINARY_BLOATY: \
+			 Option<&[u8]> = None;",
 		);
 	}
 }

Would it be acceptable to save these changes for a future PR?

bkchr · 2021-07-19T13:00:18Z

Would it be acceptable to save these changes for a future PR?

Why?

The changes above look weird. Not sure we really need format_strings.

expenses · 2021-07-19T13:05:22Z

Would it be acceptable to save these changes for a future PR?

Why?

The changes above look weird. Not sure we really need format_strings.

We might find a way for them to not be weird. But it doesn't matter, we won't include them for now.

chevdor · 2021-07-19T13:07:15Z

@expenses I think we can find small details to fix for such a PR pretty much forever :) I also added a comment but overall, I find the result very positive.

Of course, when it comes to formattings, the merging of this PR will generate extra work (potentially) subsequent changes to the standard are a bit of a pain, especially for open PRs that will need to rebase and fix a bunch of conflicts.

In short, if we can be 90% happy (and that's the case AFAIC) with the current PR, I think we should freeze its state and share the agreed config for open PRs to get ready.

I would recommend to:

freeze the state of the PR
announce the upcoming change and let PR authors merge or prepare big PRs before we merge this one

TriplEight · 2021-07-19T13:09:04Z

Do you guys want it as a blocking check in CI or is it good enough, for now, to have it launched locally?
Normally it's both.

expenses · 2021-07-19T13:10:06Z

Okay great. Yeah @chevdor that's a good idea. I think that with the number of approvals we've got so far, we can go ahead and at least commit the rustfmt.toml. We can actually format the code in other PRs, as it only requires a single cargo +nightly fmt.

kianenigma · 2021-07-19T13:11:15Z

format_strings = true

Then let's get rid of format_strings = true, the other two pretty good IMO.

expenses · 2021-07-19T13:11:48Z

Do you guys want it as a blocking check in CI or is it good enough, for now, to have it launched locally?
Normally it's both.

We only want this in CI once we've done the initial formatting of the code.

expenses · 2021-07-19T13:12:24Z

bot merge

ghost · 2021-07-19T13:12:28Z

Waiting for commit status.

drahnr · 2021-07-19T15:43:50Z

Quickly attempt to use the used toml in substrate, filed this upstream issue for normaliz_comments rust-lang/rustfmt#4909

expenses added this to In progress in 🧬✨ – Client Improvements Jun 1, 2021

github-actions bot added the A0-please_review Pull request needs code review. label Jun 1, 2021

expenses added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jun 1, 2021

expenses marked this pull request as draft June 1, 2021 14:36

expenses commented Jun 3, 2021

View reviewed changes

utils/fork-tree/src/lib.rs Outdated Show resolved Hide resolved

expenses marked this pull request as ready for review June 3, 2021 12:24

expenses requested review from pepyakin, rphmeier, kianenigma, thiolliere, tomaka, folsen and drahnr June 3, 2021 12:29

kianenigma reviewed Jun 3, 2021

View reviewed changes

rustfmt.toml Outdated Show resolved Hide resolved

shawntabrizi reviewed Jun 3, 2021

View reviewed changes

utils/build-script-utils/src/version.rs Outdated Show resolved Hide resolved

shawntabrizi reviewed Jun 3, 2021

View reviewed changes

utils/frame/rpc/support/src/lib.rs Outdated Show resolved Hide resolved

expenses requested a review from coriolinus June 3, 2021 13:53

burdges reviewed Jun 3, 2021

View reviewed changes

utils/browser/src/lib.rs Outdated Show resolved Hide resolved

Satoshi-Kusumoto mentioned this pull request Jun 4, 2021

Add style checks litentry/litentry-pallets#13

Closed

drahnr reviewed Jun 7, 2021

View reviewed changes

utils/build-script-utils/src/git.rs Outdated Show resolved Hide resolved

drahnr reviewed Jun 7, 2021

View reviewed changes

utils/build-script-utils/src/lib.rs Outdated Show resolved Hide resolved

drahnr reviewed Jun 7, 2021

View reviewed changes

gavofyork approved these changes Jul 19, 2021

View reviewed changes

bkchr removed the A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). label Jul 19, 2021

drahnr approved these changes Jul 19, 2021

View reviewed changes

chevdor approved these changes Jul 19, 2021

View reviewed changes

Unformat utils

b305c72

expenses changed the title ~~Add a rustfmt.toml and run cargo fmt on utils~~ Add a rustfmt.toml Jul 19, 2021

ghost merged commit 04f6f68 into master Jul 19, 2021

ghost deleted the ashley-rustfmt branch July 19, 2021 13:43

This was referenced Jul 19, 2021

use substrate convention for editorconfig substrate-developer-hub/substrate-node-template#192

Closed

Match rustfmt used in Substrate substrate-developer-hub/substrate-parachain-template#60

Closed

expenses mentioned this pull request Jul 19, 2021

Run cargo fmt on infrequently updated source files #9381

Closed

drahnr mentioned this pull request Jul 19, 2021

add rustfmt toml paritytech/polkadot#3491

Merged

This was referenced Jul 19, 2021

Include the rustfmt.toml when generating the node-template #9386

Closed

unset all defaults in .editorconfig for *.rs #9387

Closed

Satoshi-Kusumoto mentioned this pull request Jul 20, 2021

Feature/rust fmt litentry/litentry-parachain#62

Merged

gilescope moved this from In progress to Done in 🧬✨ – Client Improvements Jul 26, 2021

nuke-web3 mentioned this pull request Jul 29, 2021

Add rustfmt.toml polkadot-evm/frontier#431

Closed

gilescope mentioned this pull request Sep 29, 2022

Member Request polkadot-fellows/seeding#26

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `rustfmt.toml` #8982

Add a `rustfmt.toml` #8982

expenses commented Jun 1, 2021

TriplEight commented Jun 1, 2021

expenses commented Jun 3, 2021

expenses commented Jun 3, 2021

burdges Jun 3, 2021

gilescope Jul 14, 2021

chevdor Jul 19, 2021

burdges commented Jun 3, 2021

expenses commented Jun 4, 2021

drahnr Jun 7, 2021

expenses Jun 7, 2021

coriolinus Jun 7, 2021

expenses Jun 7, 2021

kianenigma commented Jul 19, 2021 •

edited

expenses commented Jul 19, 2021 •

edited

bkchr commented Jul 19, 2021

expenses commented Jul 19, 2021

chevdor commented Jul 19, 2021

TriplEight commented Jul 19, 2021

expenses commented Jul 19, 2021

kianenigma commented Jul 19, 2021

expenses commented Jul 19, 2021

expenses commented Jul 19, 2021

ghost commented Jul 19, 2021

drahnr commented Jul 19, 2021

Add a rustfmt.toml #8982

Add a rustfmt.toml #8982

Conversation

expenses commented Jun 1, 2021

TriplEight commented Jun 1, 2021

expenses commented Jun 3, 2021

expenses commented Jun 3, 2021

burdges Jun 3, 2021

Choose a reason for hiding this comment

gilescope Jul 14, 2021

Choose a reason for hiding this comment

chevdor Jul 19, 2021

Choose a reason for hiding this comment

burdges commented Jun 3, 2021

expenses commented Jun 4, 2021

drahnr Jun 7, 2021

Choose a reason for hiding this comment

expenses Jun 7, 2021

Choose a reason for hiding this comment

coriolinus Jun 7, 2021

Choose a reason for hiding this comment

expenses Jun 7, 2021

Choose a reason for hiding this comment

kianenigma commented Jul 19, 2021 • edited

expenses commented Jul 19, 2021 • edited

bkchr commented Jul 19, 2021

expenses commented Jul 19, 2021

chevdor commented Jul 19, 2021

TriplEight commented Jul 19, 2021

expenses commented Jul 19, 2021

kianenigma commented Jul 19, 2021

expenses commented Jul 19, 2021

expenses commented Jul 19, 2021

ghost commented Jul 19, 2021

drahnr commented Jul 19, 2021

Add a `rustfmt.toml` #8982

Add a `rustfmt.toml` #8982

kianenigma commented Jul 19, 2021 •

edited

expenses commented Jul 19, 2021 •

edited