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

Fix clippy warnings and add Cargo-clippy check to CI #12

Open
Mart-Bogdan opened this issue Dec 23, 2022 · 0 comments
Open

Fix clippy warnings and add Cargo-clippy check to CI #12

Mart-Bogdan opened this issue Dec 23, 2022 · 0 comments

Comments

@Mart-Bogdan
Copy link
Contributor

Mart-Bogdan commented Dec 23, 2022

When cargo clippy is run against our source we are getting following warnings:

[winnie@bm-linux dirstat-rs]$ cargo clippy
    Checking dirstat-rs v0.3.8 (/home/winnie/work/dirstat-rs)
warning: this expression creates a reference which is immediately dereferenced by the compiler
  --> src/lib.rs:28:24
   |
28 |             .unwrap_or(&OsStr::new("."))
   |                        ^^^^^^^^^^^^^^^^ help: change this to: `OsStr::new(".")`
   |
   = note: `#[warn(clippy::needless_borrow)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

warning: `dirstat-rs` (lib) generated 1 warning
warning: this expression creates a reference which is immediately dereferenced by the compiler
  --> src/bin/main.rs:27:41
   |
27 |     let file_info = FileInfo::from_path(&target_dir, config.apparent)?;
   |                                         ^^^^^^^^^^^ help: change this to: `target_dir`
   |
   = note: `#[warn(clippy::needless_borrow)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

warning: this expression creates a reference which is immediately dereferenced by the compiler
  --> src/bin/main.rs:44:36
   |
44 |             DiskItem::from_analyze(&target_dir, config.apparent, volume_id)?
   |                                    ^^^^^^^^^^^ help: change this to: `target_dir`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

warning: this expression creates a reference which is immediately dereferenced by the compiler
  --> src/bin/main.rs:62:21
   |
62 |     show_item(item, &info, buffer)?;
   |                     ^^^^^ help: change this to: `info`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

warning: `format!` in `write!` args
  --> src/bin/main.rs:90:5
   |
90 |     write!(buffer, " {} ", format!("{:.2}%", info.fraction))?;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(clippy::format_in_format_args)]` on by default
   = help: combine the `format!(..)` arguments with the outer `write!(..)` call
   = help: or consider changing `format!` to `format_args!`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#format_in_format_args

warning: manual `RangeInclusive::contains` implementation
   --> src/bin/main.rs:199:8
    |
199 |     if num >= 0.0 && num <= 100.0 {
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `(0.0..=100.0).contains(&num)`
    |
    = note: `#[warn(clippy::manual_range_contains)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains

warning: `dirstat-rs` (bin "ds") generated 5 warnings

Those warnings can be easily fixed.

Also CI check should be added.

as actions-rs/clippy don't work at all anymore, and actions-rs/clippy-check is unable to process externall PRs for now we can just run clippy as exec/run step in workflow. And fail if we have non zero exit code. And just have errors in console, not as annotations in PR.

Unresolved question is: Should we threat them as errors, preventing PR, or just as warning, so just run check, but don't fail CI because of it?

Originally posted by @scullionw in #10 (comment)

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

No branches or pull requests

1 participant