Skip to content
This repository has been archived by the owner on Oct 13, 2023. It is now read-only.

Add rustfmt-check #22

Open
JP-Ellis opened this issue Nov 28, 2019 · 4 comments
Open

Add rustfmt-check #22

JP-Ellis opened this issue Nov 28, 2019 · 4 comments

Comments

@JP-Ellis
Copy link

Would it be difficult to adapt clippy-check to create rustfmt-check?

There is the same --mesage-format=json option for cargo fmt, though currently it is in a bit of limbo on stable (rust-lang/rustfmt#3947).

I tried forking clippy-check to create rustfmt-check, but was unable to get npm to install the right dependencies and I'm not sure how things are meant to work. You can see the 'fork' at https://github.com/JP-Ellis/rustfmt-check and a test crate using it at https://github.com/JP-Ellis/test-rustfmt-check and if I'm on the right track, help would be appreciated.

@svartalf
Copy link
Member

svartalf commented Dec 9, 2019

Hi, @JP-Ellis! Well, yes, technically, there should be no difficulties, but I can't imagine any reason to do that so far, so it would help if you could elaborate a little bit more about your case.

clippy-check provides information that can't be usually fixed by a machine, while formatting issues can be resolved with the rustfmt call (manual or automatic, see #8) usually.
In what way we could benefit more from the rustfmt-check Action instead of, let's say, implemented #8?

@JP-Ellis
Copy link
Author

Well, I can see three ways in which rustfmt can be used:

  1. Have rustfmt-check automatically create a commit and push it on top of pull requests, or automatically open pull requests for commits pushed.
  2. Have rustfmt-check issue a warning if the commit is not formatted correctly with instructions as to how this can be done (in case the pull request is from someone new to rust).
  3. Have rustfmt-check create a more extensive report detailing the changes that will happen, akin to clippy-check.

Although I personally would use the first two options the most, it might be worth adding support for the last option especially in cases where some of the code should not be reformatted and there is a missing #[rustfmt::skip], or an error in the ignore section of rustfmt.toml.

@RReverser
Copy link

provides information that can't be usually fixed by a machine

Lots (most?) of the suggestions can actually be fixed with cargo fix, so I'd argue same reasoning should apply.

And, either way, since we already have a cargo fmt action that displays issues, I'd argue that improving visibility by showing these issues inline could be great.

@mbrobbel
Copy link

I started working on an action based on the ideas presented in this issue: https://github.com/mbrobbel/rustfmt-check.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

4 participants