Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add a rustfmt.toml #8982

Merged
4 commits merged into from
Jul 19, 2021
Merged

Add a rustfmt.toml #8982

4 commits merged into from
Jul 19, 2021

Conversation

expenses
Copy link
Contributor

@expenses expenses commented Jun 1, 2021

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.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jun 1, 2021
@expenses 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 expenses marked this pull request as draft June 1, 2021 14:36
@TriplEight
Copy link
Contributor

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

@expenses
Copy link
Contributor Author

expenses commented Jun 3, 2021

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

utils/fork-tree/src/lib.rs Outdated Show resolved Hide resolved
@expenses expenses marked this pull request as ready for review June 3, 2021 12:24
@expenses
Copy link
Contributor Author

expenses commented Jun 3, 2021

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 Outdated Show resolved Hide resolved
@expenses expenses requested a review from coriolinus June 3, 2021 13:53
@@ -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),
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@burdges
Copy link

burdges commented Jun 3, 2021

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
Copy link
Contributor Author

expenses commented Jun 4, 2021

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@expenses expenses requested review from gilescope and removed request for a team July 19, 2021 10:22
@bkchr bkchr removed the A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). label Jul 19, 2021
@kianenigma
Copy link
Contributor

kianenigma commented Jul 19, 2021

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
Copy link
Contributor Author

expenses commented Jul 19, 2021

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
Copy link
Member

bkchr commented Jul 19, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

chevdor commented Jul 19, 2021

@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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

format_strings = true

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

@expenses
Copy link
Contributor Author

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 expenses changed the title Add a rustfmt.toml and run cargo fmt on utils Add a rustfmt.toml Jul 19, 2021
@expenses
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Jul 19, 2021

Waiting for commit status.

@ghost ghost merged commit 04f6f68 into master Jul 19, 2021
@ghost ghost deleted the ashley-rustfmt branch July 19, 2021 13:43
@drahnr
Copy link
Contributor

drahnr commented Jul 19, 2021

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

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.