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

chore: Format imports using rustfmt #2812

Merged
merged 11 commits into from
Oct 18, 2024
Merged

chore: Format imports using rustfmt #2812

merged 11 commits into from
Oct 18, 2024

Conversation

flub
Copy link
Contributor

@flub flub commented Oct 17, 2024

Description

This enables a strict style in rustfmt to format imports statements. It needs rust nightly for rustfmt to be able to do this.

To not make the cargo fmt from stable rust really noisy it does not add a rustfmt.toml file, instead it adds a cargo-make task to automate running this locally. This same task is used in CI to run the formatter.

The Cratem formatting will be preserved by the default configuration of rust-analyzer, which makes external contributions easier as well, most folks shouldn't be running into this formatter.

Breaking Changes

Notes & open questions

I guess we need buy-in from everyone on the team for this.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

Copy link

github-actions bot commented Oct 17, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2812/docs/iroh/

Last updated: 2024-10-17T17:41:59Z

Copy link

github-actions bot commented Oct 17, 2024

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: b43a704

@flub flub marked this pull request as ready for review October 17, 2024 16:04
@matheus23
Copy link
Member

matheus23 commented Oct 17, 2024

image
Lol
Seems like this formatting style is more line-efficient.

@flub
Copy link
Contributor Author

flub commented Oct 17, 2024

FWIW, personally I like module style. But I'd also be really happy if crate style manages to be consistently formatted. So I'm totally open to adopt that if it's preferred. I don't want to die on a hill about what formatting is used. I only want to die on the hill of consistency.

Copy link
Contributor

@divagant-martian divagant-martian left a comment

Choose a reason for hiding this comment

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

I updated the pr to not depend on nightly.

Also pushed changes that are incoming on the nightly channel but are accepted by check.

Also changed the pr to Crate, which a couple of us seem to prefer.. so obviously LGTM there.

Also did a couple tries of removing imports and asking rust-analyzer do add them back without further configuration and it behaves how we want. Also tested that cargo make format produces no diff locally.

Happy we are improving our code quality and standards

Copy link
Contributor Author

@flub flub left a comment

Choose a reason for hiding this comment

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

LGTM, let's merge this already before it gets out of date!

(can't approve my own PR)

@divagant-martian
Copy link
Contributor

divagant-martian commented Oct 18, 2024

Merging, as internal discussions point to (at least by majority) consensus

@divagant-martian divagant-martian added this pull request to the merge queue Oct 18, 2024
Merged via the queue into main with commit 8808a36 Oct 18, 2024
27 of 28 checks passed
@flub flub deleted the flub/formatting branch October 18, 2024 08:37
matheus23 pushed a commit that referenced this pull request Nov 14, 2024
## Description

This enables a strict style in rustfmt to format imports statements. It
needs rust nightly for rustfmt to be able to do this.

To not make the `cargo fmt` from stable rust really noisy it does not
add a rustfmt.toml file, instead it adds a `cargo-make` task to automate
running this locally. This same task is used in CI to run the formatter.

The `Crate`m formatting will be preserved by the default configuration
of rust-analyzer, which makes external contributions easier as well,
most folks shouldn't be running into this formatter.

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

I guess we need buy-in from everyone on the team for this.

## Change checklist

- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.

---------

Co-authored-by: Diva M <[email protected]>
Co-authored-by: Divma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants