Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clippy linting #104

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

bailey-coding
Copy link
Contributor

Relates to #13

@iolivia
Copy link
Owner

iolivia commented Feb 13, 2022

does clippy lint succeed on the current code? 😄

@bailey-coding
Copy link
Contributor Author

Thanks for the great project, by the way! I just followed it and enjoyed it a lot!

There are just a few warnings:

warning: writing `&String` instead of `&str` involves a new object where a slice will do
  --> src/audio.rs:12:64
   |
12 |     pub fn play_sound(&mut self, context: &mut Context, sound: &String) {
   |                                                                ^^^^^^^ help: change this to: `&str`
   |
   = note: `#[warn(clippy::ptr_arg)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

warning: very complex type used. Consider factoring parts into `type` definitions
  --> src/systems/event_system.rs:17:23
   |
17 |       type SystemData = (
   |  _______________________^
18 | |         Write<'a, EventQueue>,
19 | |         Write<'a, AudioStore>,
20 | |         Entities<'a>,
...  |
23 | |         ReadStorage<'a, Position>,
24 | |     );
   | |_____^
   |
   = note: `#[warn(clippy::type_complexity)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity

warning: very complex type used. Consider factoring parts into `type` definitions
  --> src/systems/input_system.rs:15:23
   |
15 |       type SystemData = (
   |  _______________________^
16 | |         Write<'a, EventQueue>,
17 | |         Write<'a, InputQueue>,
18 | |         Write<'a, Gameplay>,
...  |
23 | |         ReadStorage<'a, Immovable>,
24 | |     );
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity

warning: using `clone` on type `u32` which implements the `Copy` trait
  --> src/systems/input_system.rs:80:56
   |
80 |                         Some(id) => to_move.push((key, id.clone())),
   |                                                        ^^^^^^^^^^ help: try dereferencing it: `*id`
   |
   = note: `#[warn(clippy::clone_on_copy)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy

warning: length comparison to zero
  --> src/systems/input_system.rs:99:12
   |
99 |         if to_move.len() > 0 {
   |            ^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!to_move.is_empty()`
   |
   = note: `#[warn(clippy::len_zero)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#len_zero

warning: `rust-sokoban` (bin "rust-sokoban") generated 5 warnings
    Finished dev [unoptimized + debuginfo] target(s) in 2m 36s

I can make an MR for the !is_empty, but not sure about the complex type ones?

@iolivia
Copy link
Owner

iolivia commented Feb 13, 2022

hey, super glad to hear, thanks for all the fixes, really appreciate it! 🙏

is it possible to ignore the type complexity rule when we run clippy? maybe there is some clippy config where we can add an ignore for this specific rule without changing the code.

the rest should be fine to fix, the main thing is to make sure all the code changes stays on the same line, this is very unfortunate but cause we are using line numbers in the book if the code changes significantly it's a pain to change.

@bailey-coding
Copy link
Contributor Author

bailey-coding commented Feb 13, 2022 via email

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

Successfully merging this pull request may close these issues.

None yet

2 participants