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

[RFC] Style checking our libs with rustfmt #289

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

korken89
Copy link
Contributor

@korken89 korken89 commented Jan 8, 2019

No description provided.

@korken89 korken89 requested review from dylanmckay, jcsoo and a team as code owners January 8, 2019 20:39
@korken89
Copy link
Contributor Author

korken89 commented Jan 8, 2019

Was discussed on the WG meeting on the 2019-01-08, the logs can be of reference.

paoloteti
paoloteti previously approved these changes Jan 8, 2019
@adamgreig
Copy link
Member

I am generally 👍 on this plan but I worry about edge cases where the rustfmt style is really bad for embedded, especially (only?) how it handles svd2rust's API with code like

    rcc.ahb1enr.modify(|_, w| {
        w.crcen()
            .enabled()
            .ethmacrxen()
            .enabled()
            .ethmactxen()
            .enabled()
            .ethmacen()
            .enabled()
    });

I doubt I'm alone in preferring to have it go register().variant() per line. and the alignment seems totally off as well. I think this could be fixed with a custom rustfmt style for embedded projects, which might be worth looking in to.

On the other hand, I don't think much or any of the code maintained by the WG actually uses svd2rust crates much (only HAL implementations which tend to be in their own orgs), so it might be a total non-issue for us.

Finally much as I'd much prefer having bors just apply rustfmt when it does the merge commit, I don't think this is possible with bors at the moment, so we'd probably just have to make it a Travis check and insist contributors run rustfmt themselves.

@adamgreig adamgreig added RFC needs-decision This RFC or PR needs to be approved by the majority of reviewers before it's merged T-all labels Jan 9, 2019
@Disasm
Copy link
Member

Disasm commented Feb 24, 2019

Now we have template repo, where we can introduce and agree upon perfect rustfmt rules.

@burrbull
Copy link
Member

#rust-lang/rustfmt#3414

@david-sawatzke
Copy link

@adamgreig Rustfmt formats this correctly and it doesn't look bad. It may be worth it to adopt this style

@korken89
Copy link
Contributor Author

The question of automating this can now be part of the git hooks using https://github.com/rhysd/cargo-husky

@thejpster
Copy link
Contributor

I vote we merge this, and select the option of rejecting a PR if the author hasn't run rustfmt. We can also squash their commits to avoid formatting related noise.

@adamgreig
Copy link
Member

Just to throw another option in the mix, GitHub Actions can trigger a script on push to master that can itself push back to master, so we could have that automatically run rustfmt after each PR is merged.

I'm also happy with having Travis just reject the PR if rustfmt isn't clean, and optionally add a Husky pre-commit hook to remind authors to format code before committing.

@andre-richter
Copy link
Member

This would mean an additional commit for each fmt run? While convenient, the added noise would be unpleasant I think.

@reitermarkus
Copy link
Member

You can also push the formatting changes to a PR itself unless the author has unchecked “Allow edits by maintainers”. Then again, I think it is a reasonable assumption that someone who can write Rust code is also capable of running rustfmt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision This RFC or PR needs to be approved by the majority of reviewers before it's merged RFC T-all
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants