nannou icon indicating copy to clipboard operation
nannou copied to clipboard

Run clippy on the project

Open TeamTamoad opened this issue 2 years ago • 1 comments

Currently clippy generate tons of warning on this project. I think it's very important to fix most of these warnings to make the project more idiomatic and readable for rust developers. I'm willing to fix this issue. I want to hear from maintainers about this issue and how can i approach it. Most of the warnings can be fixed by running cargo clippy --fix. However, that could result in a very big PR. Another important piece of information to fix this issue is the current minimum supported Rust version (MSRV) of this project which I cannot seem to find.

A rough summary of the warnings can be generated by running this command.

cargo clippy 2>&1 | rg 'warning: ' | sort | uniq -c | sort -bgr
command result (The first column is a frequency)
     66 warning: redundant field names in struct initialization
     56 warning: this expression creates a reference which is immediately dereferenced by the compiler
     26 warning: an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true
      9 warning: useless conversion to the same type: `[S; 3]`
      7 warning: needless `fn main` in doctest
      7 warning: match expression looks like `matches!` macro
      6 warning: use of `unwrap_or` followed by a function call
      6 warning: explicit lifetimes given in parameter types where they could be elided (or replaced with `'_` if needed by type declaration)
      5 warning: you are using an explicit closure for copying elements
      5 warning: returning the result of a `let` binding from a block
      4 warning: unsafe function's docs miss `# Safety` section
      4 warning: this pattern creates a reference to a reference
      4 warning: this function has too many arguments (9/7)
      3 warning: using `Option.and_then(|x| Some(y))`, which is more succinctly expressed as `map(|x| y)`
      3 warning: use of `flat_map` with an identity function
      3 warning: useless conversion to the same type: `nannou_core::geom::Vec2`
      3 warning: this `if` statement can be collapsed
      3 warning: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
      2 warning: very complex type used. Consider factoring parts into `type` definitions
      2 warning: using `clone` on type `winit::window::WindowId` which implements the `Copy` trait
      2 warning: using `clone` on type `winit::event::TouchPhase` which implements the `Copy` trait
      2 warning: using `clone` on type `winit::event::MouseButton` which implements the `Copy` trait
      2 warning: using `clone` on type `winit::event::DeviceId` which implements the `Copy` trait
      2 warning: using `clone` on type `winit::dpi::PhysicalSize<u32>` which implements the `Copy` trait
      2 warning: using `clone` on type `nannou_wgpu::BlendComponent` which implements the `Copy` trait
      2 warning: use of `extend` instead of `append` for adding the full range of a second vector
      2 warning: useless conversion to the same type: `[S; 2]`
      2 warning: useless conversion to the same type: `nannou_core::geom::Vec3`
      2 warning: useless conversion to the same type: `cpal::StreamConfig`
      2 warning: this `MutexGuard` is held across an `await` point
      2 warning: this loop could be written as a `for` loop
      2 warning: this `else { if .. }` block can be collapsed
      2 warning: `if` chain can be rewritten with `match`
      1 warning: you should consider adding a `Default` implementation for `Window`
      1 warning: you should consider adding a `Default` implementation for `Path`
      1 warning: you should consider adding a `Default` implementation for `Mouse`
      1 warning: you should consider adding a `Default` implementation for `Map`
      1 warning: you should consider adding a `Default` implementation for `ButtonMap`
      1 warning: you should consider adding a `Default` implementation for `Builder`
      1 warning: you should consider adding a `Default` implementation for `Api`
      1 warning: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
      1 warning: you are using an explicit closure for cloning elements
      1 warning: using `clone` on type `winit::event::MouseScrollDelta` which implements the `Copy` trait
      1 warning: using `clone` on type `winit::event::ModifiersState` which implements the `Copy` trait
      1 warning: using `clone` on type `wgpu::Extent3d` which implements the `Copy` trait
      1 warning: using `clone` on type `u64` which implements the `Copy` trait
      1 warning: using `clone` on type `std::net::IpAddr` which implements the `Copy` trait
      1 warning: using `clone` on type `point::Point` which implements the `Copy` trait
      1 warning: using `clone` on type `i64` which implements the `Copy` trait
      1 warning: using `clone` on type `f32` which implements the `Copy` trait
      1 warning: using `clone` on type `event::Update` which implements the `Copy` trait
      1 warning: using `clone` on type `char` which implements the `Copy` trait
      1 warning: using `clone` on type `bool` which implements the `Copy` trait
      1 warning: use of `filter_map` with an identity function
      1 warning: use of `expect` followed by a function call
      1 warning: useless conversion to the same type: `rusttype::gpu_cache::Cache`
      1 warning: useless conversion to the same type: `nannou_core::prelude::Mat4`
      1 warning: unneeded unit return type
      1 warning: unneeded unit expression
      1 warning: unneeded `return` statement
      1 warning: this lifetime isn't used in the function definition
      1 warning: this import is redundant
      1 warning: this if-then-else expression returns a bool literal
      1 warning: this `if let` can be collapsed into the outer `if let`
      1 warning: this function has too many arguments (8/7)
      1 warning: this function has too many arguments (12/7)
      1 warning: this function has too many arguments (10/7)
      1 warning: redundant closure
      1 warning: redundant clone
      1 warning: `nannou_wgpu` (lib) generated 23 warnings
      1 warning: `nannou_osc` (lib) generated 9 warnings
      1 warning: `nannou_new` (bin "nannou_new") generated 1 warning
      1 warning: `nannou_mesh` (lib) generated 2 warnings
      1 warning: `nannou` (lib) generated 196 warnings
      1 warning: `nannou_laser` (lib) generated 18 warnings
      1 warning: `nannou_core` (lib) generated 33 warnings
      1 warning: `nannou_audio` (lib) generated 14 warnings
      1 warning: manual implementation of an assign operation
      1 warning: length comparison to zero
      1 warning: large size difference between variants
      1 warning: field is never read: `uniform_buffer`
      1 warning: field is never read: `sampler`
      1 warning: field is never read: `default_texture`
      1 warning: field is never read: `bind_group_layout`
      1 warning: field assignment outside of initializer for an instance created with Default::default()
      1 warning: constants have by default a `'static` lifetime
      1 warning: `cfg_attr` is deprecated for rustfmt and got replaced by tool attributes
      1 warning: build failed, waiting for other jobs to finish...

Versions rustc: 1.61 clippy: 0.1.61

TeamTamoad avatar May 27 '22 20:05 TeamTamoad

I'm not a maintainer. But to avoid such large PR's, I think it would be better to run this in every sub - crate (ie nannou_osc, nannou_core etc). And then create a PR for each.

And I also agree specifying a MSRV would be good too, to prevent compile errors that people will try to solve.

Since they both can be done on crate level, at the same time, a PR including both changes - per crate would be the way to go?

mvklingeren avatar Jul 25 '22 13:07 mvklingeren