Formatting and Linting

[naomijub]

This PR does 3 main things:

1. Includes cargo fmt and cargo clippy in CI.
2. Formatting with cargo fmt, which can be adjusted in the future with rustfmt.roml
3. Linting with cargo clippy pedantic. basic rust style

TODO:

• Cargo clippy install can be cached.
• Linting with cargo clippy perf to improve performance.
• Apply to gamepad-rs

[64kramsystem]

Pedantic may be excessive; for example, on my setup, I get:

$ cargo +stable clippy -- -W clippy::pedantic
[...]
warning: casting `f64` to `usize` may lose the sign of the value
   --> src/main.rs:307:28
    |
307 |                 ".".repeat(((get_time() * 2.0) as usize) % 4)
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |

I think that pedantic warnings are irrelevant for the build, since they're not enforced (they don't cause a build failure); in these conditions, if one's really interested, it's simpler to run clippy locally rather than checking the github action log.

There is also another aspect - since there are already many standard level warnings (e.g. <expression> == true|false, which is not common practice), I think that - if there is a decision to cleanup warnings - those should be tackled first, and only after, pedantic warnings should be activated.

[naomijub]

Well, we could always add --deny warning which would fail CI in case there is any warning. I will add another TODO to optimize clippy usage as my plan was to use it for performance as well. What do you think @saveriomiroddi ?

[64kramsystem]

Well, we could always add --deny warning which would fail CI in case there is any warning. I will add another TODO to optimize clippy usage as my plan was to use it for performance as well. What do you think @saveriomiroddi ?

Let me clarify.

Right now, without pedantic, the are already 111 warnings to fix (in master); this PR fixes around 20 (as 91 are still remaining). They're inherently more important, easier to fix, and don't generally require any discussion, when compared to the pedantic ones.

Adding pedantic warnings adds other 210 warnings, bring to a total of 321/301. Another problem of pedantic warnings is also that they're not straightforward to fix, and additionally, they may need discussion. For example this:

warning: this function has too many lines (209/100)
   --> src/main.rs:287:47
    |
287 |   async fn game(game_type: GameType, map: &str) {
    |  _______________________________________________^
288 | |     use nodes::{
289 | |         Bullets, Camera, Cannon, Crate, Curse, Decoration, FlyingCurses, Fxses, Galleon, GameState,
290 | |         Grenades, Jellyfish, KickBombs, LevelBackground, MachineGun, Mines, Muscet, Player,
...   |
536 | |     }
537 | | }
    | |_^

may require discussion, as splitting function may need to meet guidelines etc. This is a real-world example of this concept.

So, while fixing the standard warnings may be a straightforward job, fixing the pedantic ones will necessarily be considerably longer and complex. By separating the two tasks, it's possible to accomplish at least one of them (standard warnings), in a shorter time.

If warnings are set as errors now, with pedantic warnings there will be a failing build with a very long laundry list, and it will take some time and discussion to get a successful build.

Regarding the performance: AFAIK, clippy performance warnings and pedantic ones are not related - I think perf ones can be run via --clippy::perf (but I may be mistaken).
I'm a bit perplexed of putting performance on the table though, before a performance issue is found 😁.

But anyway, Fedor is the tech lead (I'm not in chage of decisions), so I'm fine with anything 😁

[naomijub]

Ok, understood your point on pedantic, would you recommend other lint strategy? Maybe clippy::correctness or clippy::complexity? I don't mind fixing the remaining complexity warnings for for example, I would actually learn a lot about the code.

[naomijub]

Is there a reason to use <expression> == true|false instead of using expression or !expression? Same for killed_player_ids.len() > 0 instead of !killed_player_ids.is_empty() and killed_player_ids.len() == 0 instead of killed_player_ids.is_empty()?

if !intersect.is_none() {
    |                            ^^^^^^^^^^^^^^^^^^^^ help: try: `intersect.is_some()`

Maybe add a method is_not_trown for node.thrown == false?

[64kramsystem]

Is there a reason to use <expression> == true|false instead of using expression or !expression? Same for killed_player_ids.len() > 0 instead of !killed_player_ids.is_empty() and killed_player_ids.len() == 0 instead of killed_player_ids.is_empty()?

if !intersect.is_none() {
    |                            ^^^^^^^^^^^^^^^^^^^^ help: try: `intersect.is_some()`

Maybe add a method is_not_trown for node.thrown == false?

This is a very good question 😆, in fact, this was one of the oddities I've first noticed in the project. I personally have no idea. I think they've been inherited, style-wise, from the original project.

[64kramsystem]

Ok, understood your point on pedantic, would you recommend other lint strategy? Maybe clippy::correctness or clippy::complexity? I don't mind fixing the remaining complexity warnings for for example, I would actually learn a lot about the code.

I think it would be a productive first step would be:

• make the CI run the standard clippy with warnings treated as error (cargo clippy -- -D warnings)
• and fix them

This way, a few objectives are achieved:

1. have the codebase reach a good level of cleanness (linter-wise)
2. prevent other warnings to be introduced in the future
3. conflicts are minimized (eg. splitting functions significantly increases the risk of conflicts)

Since this is a limited amount of work, hopefully, it can be done quickly.

After that... I think anything can be put on the table in the second step (pedantic, perf, etc.etc.). I'm personally fine with everything after the first step is completed 😄

However, this is just my proposal, and @not-fl3 may have in mind an entirely different course of action 😁

[naomijub]

So, I tried fixing the cargo clippy -- -D warnings but it seems it is just an alias to cargo clippy -- -W clippy::all -D warnings which became a loop hell of clippy things to fix. Deciding one clippy approach seems the best way to start so that the clippy::all can be done in the future.

So I would go with this PR and change it to cargo clippy -- -W clippy::complexity --deny warning for a next PR.

[64kramsystem]

cargo clippy -- -D warnings

🤯👍