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.

Sign up for GitHub

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

Jump to bottom

refactor: rust formatting #422

Merged
merged 20 commits into from
May 30, 2024
Merged

refactor: rust formatting #422

merged 20 commits into from
May 30, 2024

Conversation

kodemartin
Copy link
Contributor

@kodemartin kodemartin commented May 28, 2024 •
edited
Loading

Description of change

This is a big patch that implements the formatting rules we agreed upon.

  • It updates the rustfmt.toml
  • It formats the entire codebase to comply with the new rules. The only exception are the auto-generated crates in external-crates/move/move-execution.
  • It updates the CI jobs to run the check (rust.yml, external.yml).
  • It updates scripts/execution_layer.py to use the nightly rustfmt (@fijter please have a look)
  • It skips rustfmt where it mangles with the open_rpc macro in sui-json-rpc-api crate.
  • It updates the README.md with Rust and formatting setup instructions.

Links to any relevant issues

Fixes #396

kodemartin added 2 commits May 28, 2024 11:54
@kodemartin
refactor: update rustfmt.toml
c687953 
According to the agreed upon Rust conventions.
@kodemartin
refactor: apply cargo +nightly fmt
e3df5e3
@kodemartin kodemartin marked this pull request as draft May 28, 2024 10:18
@kodemartin kodemartin force-pushed the l1sc/rust-formatting branch 3 times, most recently from 1c43385 to 5a23d5a Compare May 28, 2024 11:00
@kodemartin
refactor: update rustfmt job in Rust workflow
ebde051 
* See `.github/workflows/rust.yml`
@kodemartin kodemartin force-pushed the l1sc/rust-formatting branch from 5a23d5a to ebde051 Compare May 28, 2024 11:01
@PhilippGackstatter
Copy link

PhilippGackstatter commented May 28, 2024 •
edited
Loading

I would be much in favor of also adopting a consistent formatting of toml files. Here is a CI job that does that, using the dprint defaults (this is the config file). Otherwise, no one will be able to use auto formatting since there is no standard and people might be using different formatters or plugins, hence just creating annoying reformats in PRs. So standardizing this also makes dev life easier, imo.

Edit: Moved to issue #431.

@kodemartin
Copy link
Contributor Author

I would be much in favor of also adopting a consistent formatting of toml files. Here is a CI job that does that, using the dprint defaults (this is the config file). Otherwise, no one will be able to use auto formatting since there is no standard and people might be using different formatters or plugins, hence just creating annoying reformats in PRs. So standardizing this also makes dev life easier, imo.

@PhilippGackstatter let's delegate that to another PR. Could you create an issue to this end with this description (perhaps elaborating a bit futher also)?

@kodemartin kodemartin self-assigned this May 28, 2024
@kodemartin kodemartin added the sc-platform Issues related to the Smart Contract Platform group. label May 28, 2024
@kodemartin kodemartin marked this pull request as ready for review May 28, 2024 13:31
@DaughterOfMars
Copy link
Contributor

Users should configure their local dev environments accordingly, by first installing the required nightly version. These are draft instructions (where should we put them?):

I would put them in the README.

PhilippGackstatter
PhilippGackstatter reviewed May 28, 2024
View reviewed changes
sui-execution/v2/sui-verifier/src/lib.rs
@@ -79,7 +79,7 @@ pub fn default_verifier_config(
max_basic_blocks_in_script: None,
max_per_fun_meter_units,
max_per_mod_meter_units,
max_idenfitier_len: protocol_config.max_move_identifier_len_as_option(), // Before protocol version 9, there was no limit
max_idenfitier_len: protocol_config.max_move_identifier_len_as_option(), /* Before protocol version 9, there was no limit */
Copy link

Choose a reason for hiding this comment

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

Do we actually want to rewrite those comments with /* */? I would prefer just // although I dislike these same-line comments anyway, because they make the line so unnecessarily long.

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, was this automatic? I've never seen rustfmt do that before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that if normalize_comments or wrap_comments is true, this is the formatting result for comments in the same line.

This seems to be contrary to what one should expect from the normalize_comments. And surely not from wrap_comments either.

I guess they don't call them unstable for nothing!

Copy link

Choose a reason for hiding this comment

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

There was this issue which was apparently resolved, but indeed, there might be an edge case with same line comments with these unstable options. I guess we just have to live with that side-effect or normalization and wrapping comments.

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 agree. We can also add a new guideline to avoid same-line comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

What? normalize_comments specifically says the opposite:

Convert /* */ comments to // comments where possible

@kodemartin kodemartin marked this pull request as draft May 29, 2024 08:54
@kodemartin
Copy link
Contributor Author

Converted to draft since there are a few issues to resolve:

  • Formatting of non-workspace members
  • Mangling of wrap_comments with sui-json-rpc-api: sui_open_rpc_macros::open_rpc allows comments above function arguments for building the docs, but it requires that they are in a single line. The formatter wraps them and thus lead to build errors. To resolve this will add #[rustfmt::skip] as necessary.

@kodemartin kodemartin marked this pull request as ready for review May 29, 2024 11:48
@kodemartin
Copy link
Contributor Author

kodemartin commented May 29, 2024 •
edited
Loading

Converted to draft since there are a few issues to resolve:

  • Formatting of non-workspace members

  • Mangling of wrap_comments with sui-json-rpc-api: sui_open_rpc_macros::open_rpc allows comments above function arguments for building the docs, but it requires that they are in a single line. The formatter wraps them and thus lead to build errors. To resolve this will add #[rustfmt::skip] as necessary.

Issues solved. See updated PR description.

fijter
fijter approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@fijter fijter left a comment

Choose a reason for hiding this comment

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

👍

@kodemartin kodemartin merged commit 7bd0a48 into develop May 30, 2024
17 of 35 checks passed
@kodemartin kodemartin deleted the l1sc/rust-formatting branch May 30, 2024 15:13
alexsporn pushed a commit that referenced this pull request Sep 6, 2024
@kodemartin
refactor: rust formatting (#422)
19a90cc 
* refactor: update rustfmt.toml

* refactor: apply cargo +nightly fmt

* refactor: update rustfmt job in Rust workflow

* refactor: fix pull_request trigger on rust.yml

* refactor: use rolling nightly toolchain

* refactor: use nightly rustfmt in scripts/execution_layer.py

* refactor: update mysticeti,external workflows

* refactor(sui-json-rpc-api): skip formatting of method argument comments

* refactor: fix rustfmt check in external workflow

* refactor: format external_crates/move

* refactor(CI): trigger external workflow on develop

* refactor: add Rust and fmt setup guidelines in README

* refactor: remove suiop-cli format override
alexsporn pushed a commit that referenced this pull request Sep 6, 2024
@kodemartin
refactor: rust formatting (#422)
4e92259 
* refactor: update rustfmt.toml

* refactor: apply cargo +nightly fmt

* refactor: update rustfmt job in Rust workflow

* refactor: fix pull_request trigger on rust.yml

* refactor: use rolling nightly toolchain

* refactor: use nightly rustfmt in scripts/execution_layer.py

* refactor: update mysticeti,external workflows

* refactor(sui-json-rpc-api): skip formatting of method argument comments

* refactor: fix rustfmt check in external workflow

* refactor: format external_crates/move

* refactor(CI): trigger external workflow on develop

* refactor: add Rust and fmt setup guidelines in README

* refactor: remove suiop-cli format override
alexsporn pushed a commit that referenced this pull request Sep 6, 2024
@kodemartin
refactor: rust formatting (#422)
1931b4d 
* refactor: update rustfmt.toml

* refactor: apply cargo +nightly fmt

* refactor: update rustfmt job in Rust workflow

* refactor: fix pull_request trigger on rust.yml

* refactor: use rolling nightly toolchain

* refactor: use nightly rustfmt in scripts/execution_layer.py

* refactor: update mysticeti,external workflows

* refactor(sui-json-rpc-api): skip formatting of method argument comments

* refactor: fix rustfmt check in external workflow

* refactor: format external_crates/move

* refactor(CI): trigger external workflow on develop

* refactor: add Rust and fmt setup guidelines in README

* refactor: remove suiop-cli format override
alexsporn pushed a commit that referenced this pull request Sep 9, 2024
@kodemartin
refactor: rust formatting (#422)
93ba77e 
* refactor: update rustfmt.toml

* refactor: apply cargo +nightly fmt

* refactor: update rustfmt job in Rust workflow

* refactor: fix pull_request trigger on rust.yml

* refactor: use rolling nightly toolchain

* refactor: use nightly rustfmt in scripts/execution_layer.py

* refactor: update mysticeti,external workflows

* refactor(sui-json-rpc-api): skip formatting of method argument comments

* refactor: fix rustfmt check in external workflow

* refactor: format external_crates/move

* refactor(CI): trigger external workflow on develop

* refactor: add Rust and fmt setup guidelines in README

* refactor: remove suiop-cli format override
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaughterOfMars DaughterOfMars DaughterOfMars approved these changes

@fijter fijter fijter approved these changes

@PhilippGackstatter PhilippGackstatter PhilippGackstatter left review comments

Assignees

@kodemartin kodemartin

Labels
sc-platform Issues related to the Smart Contract Platform group.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Task (L1SC)]: Format the repository according to new guidelines
4 participants
@kodemartin @PhilippGackstatter @DaughterOfMars @fijter