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

Format and Sort features in Cargo.toml files#14803

Merged
paritytech-processbot[bot] merged 13 commits intomasterfrom
oty-format-features
Aug 23, 2023
Merged

Format and Sort features in Cargo.toml files#14803
paritytech-processbot[bot] merged 13 commits intomasterfrom
oty-format-features

Conversation

@ggwpez
Copy link
Member

@ggwpez ggwpez commented Aug 21, 2023 •
edited
Loading

Changes:

  • Canonicalize (sort+format) the features in all Cargo.toml files.
  • Add an experimental CI check that all Cargo.tomls are formatted.
  • Update style guide to mention the auto-formatting.

Its fine if we dont merge this before the monorepo; this MR is mostly to gather feedback on the formatting.
Are you OK with the canonical format that this introduces? Eventually we should add it to bot fmt.

My only point would be that the default feature looks slightly weird, give that it normally just contains one item. On the other hand, this creates a uniform formatting for all features... WDYT?

I asserted locally that this does not alter the sha256 of the kitchensink runtime.

The CI prints a diff on error to make debugging easier:

diff --git a/primitives/runtime/Cargo.toml b/primitives/runtime/Cargo.toml
index 39d0148..adef632 100644
--- a/primitives/runtime/Cargo.toml
+++ b/primitives/runtime/Cargo.toml
@@ -50,13 +50,13 @@ std = [
 	"log/std",
+	# hi
+	"rand",
 	"sp-arithmetic/std",
-	# hi
-	"rand",
 	"sp-core/std",

ggwpez added 4 commits August 21, 2023 20:49
@ggwpez
CI: Add feature sorting check
a5b6565 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Sort all features
461db54 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Add some mistakes
b2b1099 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Revert "Add some mistakes"
5a112a6 
This reverts commit b2b1099.
@ggwpez ggwpez marked this pull request as ready for review August 21, 2023 19:16
@ggwpez ggwpez requested review from a team August 21, 2023 19:16
@ggwpez ggwpez requested a review from a team August 21, 2023 19:16
@ggwpez ggwpez requested review from a team and koute as code owners August 21, 2023 19:16
@ggwpez ggwpez added 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 21, 2023
@ggwpez
CI job naming
d1b6500 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez requested a review from bkchr August 21, 2023 19:21
bkchr
bkchr approved these changes Aug 21, 2023
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Generally okay, but we should document on how to use the tool. Not sure we should let people copy a patch.

scripts/ci/gitlab/pipeline/test.yml Outdated
- .test-refs
script:
- cargo install zepter --locked --version 0.9.2 -q -f --no-default-features && zepter --version
- zepter format features --workspace --fix
Copy link
Member

@bkchr bkchr Aug 21, 2023

Choose a reason for hiding this comment

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

The tool should be documented in the contribution guides or similar and how to use it

Copy link
Contributor

@yjhmelody yjhmelody Aug 23, 2023

Choose a reason for hiding this comment

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

Better to add a bash script and run it in ci for local dev?

Copy link
Member Author

@ggwpez ggwpez Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I added some docs to the STYLE_GUIDE and mention it in the CI output.

WDYM? Running zepter format features (the --workspace and --fix flag are implicit now) is not much longer than invoking a bash script.

KiChjang
KiChjang reviewed Aug 21, 2023
View reviewed changes
liamaharon
liamaharon approved these changes Aug 22, 2023
View reviewed changes
juangirini
juangirini approved these changes Aug 22, 2023
View reviewed changes
Copy link
Contributor

@juangirini juangirini left a comment

Choose a reason for hiding this comment

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

❤️

ggwpez added 4 commits August 23, 2023 12:01
@ggwpez
Add oneliner formatting
0343b66 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Explain tool
55b025d 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Use latest version
ad6486a 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Better erorr message
7f8d2a7 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
altaua
altaua approved these changes Aug 23, 2023
View reviewed changes
@ggwpez
Use --check option
313b49b 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@paritytech-ci paritytech-ci requested a review from a team August 23, 2023 13:18
@ggwpez
Messed up the merge commit...
d98e1e3 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member Author

ggwpez commented Aug 23, 2023

bot merge

@paritytech-processbot
Copy link

paritytech-processbot bot commented Aug 23, 2023

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit b7c18f1 into master Aug 23, 2023
@paritytech-processbot paritytech-processbot bot deleted the oty-format-features branch August 23, 2023 14:21
ggwpez added a commit to ggwpez/zepter that referenced this pull request Aug 23, 2023
@ggwpez
Require to pass on Substrate master
475a7a8 
paritytech/substrate#14803 should ensure that
Substrate master stays formatted. We can therefore check this here
already to ensure to not break it.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez mentioned this pull request Aug 23, 2023
Merged
ggwpez added a commit to ggwpez/zepter that referenced this pull request Aug 23, 2023
@ggwpez
Require to pass on Substrate master (#25)
bba51e9 
paritytech/substrate#14803 should ensure that
Substrate master stays formatted. We can therefore check this here
already to ensure to not break it.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez mentioned this pull request Aug 27, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@athei athei Awaiting requested review from athei athei is a code owner

@acatangiu acatangiu Awaiting requested review from acatangiu acatangiu is a code owner

@sorpaas sorpaas Awaiting requested review from sorpaas

@andresilva andresilva Awaiting requested review from andresilva

@jsidorenko jsidorenko Awaiting requested review from jsidorenko

@cheme cheme Awaiting requested review from cheme cheme is a code owner

@koute koute Awaiting requested review from koute koute is a code owner

7 more reviewers

@KiChjang KiChjang KiChjang left review comments

@yjhmelody yjhmelody yjhmelody left review comments

@juangirini juangirini juangirini approved these changes

@bkchr bkchr bkchr approved these changes

@liamaharon liamaharon liamaharon approved these changes

@alvicsam alvicsam alvicsam approved these changes

@altaua altaua altaua approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ggwpez @KiChjang @juangirini @bkchr @yjhmelody @liamaharon @alvicsam @altaua