[WIP] Styling with rustfmt #286

gnunicorn · 2018-07-06T14:08:39Z

This is an example, how our code base would be reformatted if we applied rustfmt, with a configuration as close to the new styleguide as possible. This isn't meant to be merge but a discussion proposal.

I'd also like to address to following, when taking into consideration whether we want to move forward with this:

Every time rustfmt gets updated the CI will break
While that used to be true, there are a few things that have changed recently, which make me believe this isn't going to be as much of a problem in the future:

fmt has moved into an RFC process and released new-features behind a flag first as with the proper release process, which I believe to go much slower, too
as we need that flag set though, we have a rather complete configuration file with all options set to their current default, thus changing defaults would not effect us
you can now pin to a specific fmt version, thus if this really becomes an issue, we could pin to the current version and make intended formatting upgrades only when we want to

Comparing this to the styleguide:

Indent levels should be greater than 5 only in exceptional circumstances and certainly no greater than 8. If they are greater than 5, then consider using let or auxiliary functions in order to strip out complex inline expressions.

Can't be addressed with rustfmt, but maybe with clippy? I am very fond of this and would really like to have a tool ensuring we adhere to it!

Lines should be longer than 80 characters long only in exceptional circumstances and certainly no longer than 120. For this purpose, tabs are considered 4 characters wide.

This goes for a max_width of 100 with tabs and tabs considered 4 chars. Intentionally longer lines could be marked for rustfmt to ignore but that hasn't been done.

where is indented, and its items are indented one further

This isn't a configuration option at the moment, however the indentation-rules around where provided within the current version are coming pretty close imho

Blocks should not be used unnecessarily.

This has been deactivated here, however the "unnecessary" is another thing for clippy I guess....

gnunicorn · 2018-07-06T14:11:01Z

demo/cli/src/lib.rs

-use demo_runtime::{Block, BlockId, UncheckedExtrinsic, GenesisConfig,
-	ConsensusConfig, CouncilConfig, DemocracyConfig, SessionConfig, StakingConfig,
-	TimestampConfig};
+use demo_runtime::{


It is only possible to configure rustfmt (at the moment) to either have importans in the first line and then match up the second line to it (imports_indent = "Visual") or make one-tabbed blocks like this (imports_indent = "Block")

gnunicorn · 2018-07-06T14:13:14Z

demo/executor/src/lib.rs


-	const BLOATY_CODE: &[u8] = include_bytes!("../../runtime/wasm/target/wasm32-unknown-unknown/release/demo_runtime.wasm");
-	const COMPACT_CODE: &[u8] = include_bytes!("../../runtime/wasm/target/wasm32-unknown-unknown/release/demo_runtime.compact.wasm");
+	const BLOATY_CODE: &[u8] = include_bytes!(


In practice I'd prefix these lines with #[cfg_attr(rustfmt, rustfmt_skip)] as I consider this a valid case of going over the char-limit without problems.

I agree that this is a valid case for long lines. I also think having to resort to #[cfg… is adding noise that we do not want. Close to being a show-stopper to me. :/

well, these lines violate the rules even outside of rustfmt: it does violate the 100char rule, including the tab the second even violates the 120chars rule (134chars), which is formulated quite strictly... so a line like this, therefore, should also be flagged in a review with our current style guide - this isn't even rustfmt specific...

either we don't consider the rules as strict or we break the lines, I don't see any other options. And we don't want the rule to be strict, explicitly stating that we are okay with breaking the rule here, sounds like the proper way to do it. What is your proposal, @dvdplm ?

gnunicorn · 2018-07-06T14:15:23Z

demo/executor/src/lib.rs

-		}).collect::<Vec<_>>();
-
-		let extrinsics_root = ordered_trie_root(extrinsics.iter().map(Slicable::encode)).0.into();
+		let extrinsics = extrinsics


our house style of keeping some chained methods in the first line and wrap for a bigger block isn't really something that I was able to configure in rustfmt. While I do prefer our house style here, I'd not complain about the rustfmt style either.

This is one of the rules where I felt rustfmt was just too inflexible when I tried it the last time. Apparently nothing has changed there. :/

gnunicorn · 2018-07-06T14:17:27Z

polkadot/api/src/full.rs

+impl<B: LocalBackend<Block>> BlockBuilder
+	for ClientBlockBuilder<B, LocalCallExecutor<B, NativeExecutor<LocalDispatch>>, Block>
+where
+	::client::error::Error:


this is an example of how the where indentation works in rustfmt within the context and I think this is a decent way of addressing that...

I think it's worse than the original.

But is your complaint about where being at the beginning of the line or because it is breaking the line prior? Considering that this line, too, is a whopping 128chars, it is violation even if we adhered to the current style guide and the current style guide doesn't really say where for needs to break here or how this case should be considered.

gnunicorn · 2018-07-06T14:20:53Z

polkadot/collator/src/lib.rs


 /// Parachain context needed for collation.
 ///
 /// This can be implemented through an externally attached service or a stub.
 pub trait ParachainContext {
 	/// Produce a candidate, given the latest ingress queue information.
-	fn produce_candidate<I: IntoIterator<Item=(ParaId, Message)>>(
+	fn produce_candidate<I: IntoIterator<Item = (ParaId, Message)>>(


I opted here for the spaces (as I felt that this was more common in our code base), but we could switch this to type_punctuation_density

dvdplm

Thank you for doing this. I was hoping that rustfmt would have become more flexible than this, or at the very least would let us pick a subset of formatting rules we like and disable everything else.

dvdplm · 2018-07-06T14:39:23Z

demo/executor/src/lib.rs

-		}).collect::<Vec<_>>();
-
-		let extrinsics_root = ordered_trie_root(extrinsics.iter().map(Slicable::encode)).0.into();
+		let extrinsics = extrinsics


This is one of the rules where I felt rustfmt was just too inflexible when I tried it the last time. Apparently nothing has changed there. :/

dvdplm · 2018-07-06T14:40:35Z

polkadot/api/src/full.rs

+impl<B: LocalBackend<Block>> BlockBuilder
+	for ClientBlockBuilder<B, LocalCallExecutor<B, NativeExecutor<LocalDispatch>>, Block>
+where
+	::client::error::Error:


I think it's worse than the original.

gavofyork · 2018-07-29T11:23:21Z

Closing since this takes valuable build server time and is looking stale.

bkchr · 2018-09-11T11:47:23Z

Is there any chance, that a new version of this pull request could be getting merged? I would be willing, to work on this.

bddap · 2019-07-26T23:20:38Z

It's a year later and I just did a bit of digging on this. With a couple couple config tweaks, rustfmt gets close to satisfying the style guide.

Here is an example config that attempts to encode the formatting rules in the style guide.

The config above sets two options, both of which are part of stable rustfmt.

There are two cases where rustfmt conflicts with the style guide.

where clauses
explicit return statements

Examples of both cases are provided in the gist.

* Update xbridge-related fees (paritytech#281) * Adjust penalty * modify genesis config * rename treasuryaddr to counciladdr * Readd token discount and to channel logic (paritytech#284) * Readd token discount and channel share logic * Refactor a bit * Modify btc-fee (paritytech#285) Add maximum withdraw amount check * let tokenname verify more char * spot safe check (paritytech#286) * Revert "spot safe check (paritytech#286)" (paritytech#291) This reverts commit 8092ba50abf1d5646000342d9229b1d81e4b025d. * Feature/trustee rpc (paritytech#293) * Add 'chainx_getTrusteeAddress' rpc * Delete dead code * change verifyaddr rpc return true/false * Fix merge conflicts * token discount (paritytech#313) * token discount * adjust * fix bug * Fix token_discount in genesis_config * Rebuild wasm and replace wasm in cli/src/chainx_runtime_wasm.compact.wasm

* change option<()> to bool in runtime storage * some tip

Prepare the fraud proof implementation

gnunicorn added 2 commits July 6, 2018 15:44

adding rustfmt.toml

f0b945c

Rustfmt our codebase

0b778c2

gnunicorn commented Jul 6, 2018

View reviewed changes

dvdplm reviewed Jul 6, 2018

View reviewed changes

gavofyork closed this Jul 29, 2018

cecton mentioned this pull request Sep 8, 2020

cargo fmt code #6194

Closed

liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021

change option<()> to bool in runtime storage (paritytech#286)

cfa8263

* change option<()> to bool in runtime storage * some tip

liuchengxu added a commit to autonomys/substrate that referenced this pull request Jun 3, 2022

Merge pull request paritytech#286 from subspace/prepare-fraud-proof

e127fd6

Prepare the fraud proof implementation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Styling with rustfmt #286

[WIP] Styling with rustfmt #286

gnunicorn commented Jul 6, 2018 •

edited

Loading

gnunicorn Jul 6, 2018

gnunicorn Jul 6, 2018

dvdplm Jul 6, 2018

gnunicorn Jul 8, 2018

gnunicorn Jul 6, 2018

dvdplm Jul 6, 2018

gnunicorn Jul 6, 2018

dvdplm Jul 6, 2018

gnunicorn Jul 8, 2018

gnunicorn Jul 6, 2018

dvdplm left a comment

dvdplm Jul 6, 2018

dvdplm Jul 6, 2018

gavofyork commented Jul 29, 2018

bkchr commented Sep 11, 2018

bddap commented Jul 26, 2019

[WIP] Styling with rustfmt #286

[WIP] Styling with rustfmt #286

Conversation

gnunicorn commented Jul 6, 2018 • edited Loading

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

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented Jul 29, 2018

bkchr commented Sep 11, 2018

bddap commented Jul 26, 2019

gnunicorn commented Jul 6, 2018 •

edited

Loading