Should rustfmt and clippy checks be added to the CI?

[ZapAnton]

The README for submitting PR states that the submitted code should be formatted with rustfmt.

Since there is already a script that is used to format existing exercises, should it be used in the Travis CI to check the format of the submitted code?

Also as of Rust 1.29 the cargo clippy command works on stable. Should it also be used in Travis to ensure that the submitted code is idiomatic?

[petertseng]

clippy would be exciting. Might be time to bring back #445 . As of this moment I can't get it to run on the tests, so I guess it'd just be the examples and stubs? That's a bit of a shame. Want to hear solutions.

[ZapAnton]

cargo clippy --all-targets seems to be working on the whole crate

[ZapAnton]

I have run a simple script over the existing exercises:

   1   │ #!/bin/bash
   2   │ 
   3   │ for dir in exercises/*; do
   4   │         (
   5   │                 cd "$dir"
   6   │ 
   7   │                 cargo clippy --all-targets
   8   │         )
   9   │ done

clippy managed to check the src/lib.rs and tests/exercise-name.rs but could not check example.rs.
Perhaps another point in favor of #658?

[petertseng]

I have great news to share: Clippy is now being enforced by CI, on the tests only.

Yet to be done is:

• Clippy on the stubs (won't be able to do this for stubs that are allowed to not compile, but can be considered for stubs that do)
• Clippy on the example solutions (pretty low priority IMO, and I'd probably close this issue even if that was not done)
• rustfmt

[petertseng]

Regarding example solutions: with the merging of #1009, now we enforce that there are no Clippy errors in the example solutions, but we still allow Clippy warnings.

[petertseng]

Now #1011 enforces that we have no Clippy errors in the stubs. So here is a recap of what CI is now enforcing:

• Clippy on tests: No warnings or errors.
• Clippy on stubs: If stub is allowed to not compile, no enforcement (if it can't compile, Cargo won't be able to check it either). Otherwise: No errors. Warnings still allowed and are printed. Last known list of warnings in GitHub Actions: Run Clippy on stubs #1011
• Clippy on example solutions: No errors. Warnings still allowed and are silently swallowed. Last known list of warnings in DO NOT MERGE: Deny clippy warnings in example solutions #1012.

TODO:

• rustfmt

Regarding where to go from here:

• I don't consider the example solutions a high priority. So I'd leave them as they are right now.
• The stubs might have some value, but some of them such as "has a public len method but no corresponding is_empty method" would imply adding new tests, and I'm not sure how much value the "you should consider adding a Default implementation" has. So I'm mostly content with leaving things as they currently are, but we should discuss.
• Rustfmt is probably good to have. Existing script might already work without much modification, just haven't taken a look.

[petertseng]

Rustfmt has been added to CI. Now I am considering what, if anything, to do about the remaining Clippy warnings in examples and stubs.

One approach I'm thinking of: add attributes that allow only the specific warnings that currently exist, and deny all others. For the example solutions the attributes can probably just be added in directly. For the stubs, since it'd probably be weird to distribute the attributes to students, so I'm thinking it's possible to add the attributes only at test time like the deny warnings attributes are. This approach would prevent further warnings from being added, and the current ones can either be kept or not, depending on what is appropriate. I will give it some more thought in the coming days.