scipopt / russcip

Rust interface for SCIP
https://crates.io/crates/russcip
Apache License 2.0
37 stars 11 forks source link

Running Clippy + `rustfmt` and add them to CI #121

Closed mohammadfawaz closed 8 months ago

mohammadfawaz commented 8 months ago

When I was working on #120, I noticed that Clippy and rustfmt are not run in this project. I didn't run them in #120 to avoid polluting the PR but I thought I'd do them in a separate PR. Feel free to reject this but I've personally always found that rustfmt and Clippy highly improve the quality of the code in a Rust project.

codecov[bot] commented 8 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (431132b) 76.05% compared to head (9ab9b60) 76.02%.

Files Patch % Lines
src/scip.rs 50.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #121 +/- ## ========================================== - Coverage 76.05% 76.02% -0.03% ========================================== Files 13 13 Lines 1737 1739 +2 ========================================== + Hits 1321 1322 +1 - Misses 416 417 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mohammadfawaz commented 8 months ago

@mmghannam what do you suggest I do regarding the code coverage failures?

mmghannam commented 8 months ago

@mmghannam what do you suggest I do regarding the code coverage failures?

Well, they just failed because rustfmt sperated a method call to 3 lines instead of 1, nothing to worry about. So in this case it's easy we just ignore it 😄

mohammadfawaz commented 8 months ago

Thanks a lot @mohammadfawaz! This would improve the future contributions of this repo a whole lot! One thing I'm concerned about is the need to add this #[allow(clippy::too_many_arguments)] before some function definitions for which all arguments are necessary I believe and it bloats the repo a bit. Anyways I believe this does more good than harm in general.

Good point regarding the annoying annotation. We can instead add the following to Cargo.toml:

[lints.clippy]
too_many_arguments = "allow"

I'll open a separate issue to track that.