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

Do rustfmt on the codebase #619

Closed
wants to merge 2 commits into from
Closed

Conversation

hellow554
Copy link
Contributor

This is somewhat a sequel of #421

I removed the rustfmt file which ignores all files and did cargo fmt on the codebase.

I know, that you have concerns about PRs that will get a merge conflict once this landed.

I'm not sure about which ones we are talking since there are PRs from 2020 and older, which might have a conflict anyway.

So, what do people need to do: just run cargo fmt on their codebase, rebase it with master and everything should just work™.

This would help to have a consistent code style as well as a readable one in some places.

@@ -14,6 +14,7 @@ jobs:
steps:
- uses: actions/checkout@v2
- uses: dtolnay/rust-toolchain@1.36.0
- run: cargo fmt --all -- --check
Copy link
Contributor

Choose a reason for hiding this comment

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

would it not be better to add a separate CI target for rustfmt? and why use the MSRV for running the formatting?

something like-

  fmt:
    name: format
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - uses: dtolnay/rust-toolchain@nightly
        with:
          components: rustfmt
      - uses: actions-rs/cargo@v1
        with:
          command: fmt
          args: --all -- --check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if introducing yet another toolchain is worth here. You can use the stable chain instead, which already exists.

But I'm down for using checkout@3 as well as dtolnay's rust-toolchain.
I have a weird connection with actions-rs, because some of their actions are no longer maintained, but cargo looks good to me, so yeah, that's worth a rewrite in the github workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if introducing yet another toolchain is worth here

in what sense? I would personally split these into separate targets even if they both used the stable toolchain

@danieleades
Copy link
Contributor

I'm fully in support of this PR!

The best time to introduce formatting is yesterday. The second best time is today.

I don't think the original motivation of temporarily disabling rustfmt to avoid PR conflicts makes sense, unless you had some foreknowledge that future PRs were going to dry up for some reason. That's demonstrably not the case here. Instead, i think it's better to just rip the band-aid off.

@jswrenn
Copy link
Member

jswrenn commented Jun 14, 2023

I'm going to tackle this myself as part of #702.

@jswrenn jswrenn closed this Jun 14, 2023
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.

3 participants