Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: formatting #184

Merged
merged 18 commits into from
Aug 22, 2024
Merged

style: formatting #184

merged 18 commits into from
Aug 22, 2024

Conversation

evilrobot-01
Copy link
Collaborator

@evilrobot-01 evilrobot-01 commented Aug 13, 2024

Improves formatting and checks through standardisation. The main goal is automation as much as possible, whether possible in the dev environment or automated via CI.

CI

This draft adds the following checks only (i.e. no auto-fixes).

  • formatting checks of Rust files using Nightly, as it provides a number of additional useful configuration options (.rustfmt.toml)
    • can be run locally via cargo +nightly fmt --all or configured in editor/ide or commit hooks
  • formatting checks of Cargo manifests using taplo
    • can be run locally with taplo format --check and taplo format to fix
  • formatting and checks of features using zepter
    • can be run locally with zepter format features and zepter format features --fix to fix

Manifest formatting is generally alphabetically by 'region', inline with Rust imports which are alphabetical and grouped by source (std library, external, internal). I feel the latter does indeed have some benefit over one large grouping. Subjective grouping of imports can make it difficult for others to immediately understand how things should be grouped and applying accordingly. Using a standard approach eliminates this concern.

Local

A pre-push git hook is included at ./githooks and needs to be manually installed via the instructions in the readme at the same location. It currently only runs the Rust formatting checks as listed above, but can be extended to include the other checks noted as well.

A pre-commit hook could also be added to auto-format, but most editors/ides probably allow this to be set on save so re-running it all again on each commit might be overkill.

@evilrobot-01 evilrobot-01 force-pushed the frank/fmt branch 2 times, most recently from 64b5975 to 3e95817 Compare August 13, 2024 14:39
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 6.40%. Comparing base (c4e2a6d) to head (8f4c831).

Files Patch % Lines
node/src/command.rs 0.00% 16 Missing ⚠️
runtime/devnet/src/config/xcm.rs 0.00% 2 Missing ⚠️
runtime/testnet/src/config/xcm.rs 0.00% 2 Missing ⚠️
@@          Coverage Diff          @@
##            main    #184   +/-   ##
=====================================
  Coverage   6.40%   6.40%           
=====================================
  Files         27      27           
  Lines       1997    1997           
  Branches    1997    1997           
=====================================
  Hits         128     128           
  Misses      1869    1869           
Files Coverage Δ
node/src/rpc.rs 0.00% <ø> (ø)
node/src/service.rs 0.00% <ø> (ø)
primitives/src/storage_keys.rs 0.00% <ø> (ø)
runtime/common/src/lib.rs 0.00% <ø> (ø)
runtime/devnet/src/config/contracts.rs 60.00% <ø> (ø)
runtime/devnet/src/config/proxy.rs 0.00% <ø> (ø)
runtime/devnet/src/extensions.rs 0.00% <ø> (ø)
runtime/devnet/src/lib.rs 3.89% <ø> (ø)
runtime/devnet/src/weights/paritydb_weights.rs 73.33% <ø> (ø)
runtime/devnet/src/weights/rocksdb_weights.rs 73.33% <ø> (ø)
... and 9 more

@Daanvdplas
Copy link
Collaborator

Daanvdplas commented Aug 14, 2024

All looks perfect to me! cargo +nightly fmt already seems to do a lot of things! Zepter and Taplo will definitely come in handy for the pop cli repo as well!

Again, this gives me so much piece of mind! Thank you very much ❤️

Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

Very excited! Thanks for the effort you put into this. I think it will have a very positive impact on our development output as a team!

.rustfmt.toml Show resolved Hide resolved
Copy link
Collaborator

@al3mart al3mart left a comment

Choose a reason for hiding this comment

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

I have open a PR to extend the githooks with TOML and features formating in #229.

Happy to rebase that PR if we want to merge this first! 👍

@evilrobot-01
Copy link
Collaborator Author

I have open a PR to extend the githooks with TOML and features formating in #229.

Happy to rebase that PR if we want to merge this first! 👍

As this has been sitting for a week, my suggestion would be to get this merged and then treat that as a separate PR. I have not tested that locally so not sure how much additional time it adds running the checks prior to a push?

runtime/testnet/src/config/xcm.rs Outdated Show resolved Hide resolved
Co-authored-by: Tin Chung <56880684+chungquantin@users.noreply.github.com>
@evilrobot-01 evilrobot-01 merged commit 8072a47 into main Aug 22, 2024
8 checks passed
@evilrobot-01 evilrobot-01 deleted the frank/fmt branch August 22, 2024 23:07
chungquantin added a commit that referenced this pull request Sep 6, 2024
Co-authored-by: Tin Chung <56880684+chungquantin@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants