search

actions-rs / meta

🦀 GitHub Actions for Rust - recipes, discussions, questions and ideas
https://github.com/actions-rs
Creative Commons Zero v1.0 Universal
353 stars 15 forks source link

Add `rustfmt-check` #22

Open JP-Ellis opened 4 years ago

JP-Ellis commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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