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

lint: fix lints of new Rust version #6102

Merged
merged 17 commits into from
Mar 23, 2024
Merged

lint: fix lints of new Rust version #6102

merged 17 commits into from
Mar 23, 2024

Conversation

Krysztal112233
Copy link
Contributor

Fix new lints of clippy.

@Krysztal112233
Copy link
Contributor Author

Heres the action's log about failure, but it doesn't happen on my Debian testing.

   error: unused import: `set_symlink_file_times`
   --> tests/by-util/test_touch.rs:8:16
    |
  8 | use filetime::{set_symlink_file_times, FileTime};
    |                ^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D unused-imports` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unused_imports)]`
  
  error: could not compile `coreutils` (test "tests") due to 1 previous error
  Error: ERROR: `cargo clippy`: unused import: `set_symlink_file_times` (file:'tests/by-util/test_touch.rs', line:8)
       Removed 2866 files, 693.6MiB total
  exec shell: bash run.sh showDebugInfo

It only happend on freebsd target I think.

@Krysztal112233 Krysztal112233 marked this pull request as ready for review March 21, 2024 16:14
@Krysztal112233 Krysztal112233 changed the title lint: fix lint clippy::suspicious_open_options lint: fix lint of new Rust version Mar 21, 2024
@Krysztal112233 Krysztal112233 changed the title lint: fix lint of new Rust version lint: fix lints of new Rust version Mar 21, 2024
@Krysztal112233
Copy link
Contributor Author

coreutils::tests test_sort::test_output_is_input's failure may not caused by this PR after my check.

src/uu/sort/src/sort.rs Outdated Show resolved Hide resolved
@sylvestre
Copy link
Contributor

could you please fix the windows clippy warnings too as you are working on it ?
https://github.com/uutils/coreutils/actions/runs/8378663843/job/22943749149?pr=6102

@Krysztal112233
Copy link
Contributor Author

could you please fix the windows clippy warnings too as you are working on it ?
https://github.com/uutils/coreutils/actions/runs/8378663843/job/22943749149?pr=6102

I'll fix it later

src/uu/sort/src/sort.rs Outdated Show resolved Hide resolved
@Krysztal112233
Copy link
Contributor Author

Krysztal112233 commented Mar 22, 2024

Here's a lots of denied warnings about clippy::cognitive_complexity

 error: the function has a cognitive complexity of (14/10)
   --> tests\common\random.rs:254:8
    |
254 |     fn test_random_string_generate_with_delimiter_should_not_end_with_delimiter() {
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: you could split it up into multiple smaller functions
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity

and the longest is 71

error: the function has a cognitive complexity of (71/24)
   --> tests/by-util/test_env.rs:448:4
    |
448 | fn test_env_with_gnu_reference_parsing_errors() {
    |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: you could split it up into multiple smaller functions
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity
    = note: `-D clippy::cognitive-complexity` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::cognitive_complexity)]`

There are two solutions

  • Change the configuration of cognitive-complexity-threshold in .clippy.toml from 10 to 71, avoid changing the unit testing function name
  • Change from 10 to 24 and rename only one unit testing function name

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@Krysztal112233
Copy link
Contributor Author

Here's a lots of denied warnings about clippy::cognitive_complexity

 error: the function has a cognitive complexity of (14/10)
   --> tests\common\random.rs:254:8
    |
254 |     fn test_random_string_generate_with_delimiter_should_not_end_with_delimiter() {
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: you could split it up into multiple smaller functions
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity

and the longest is 71

error: the function has a cognitive complexity of (71/24)
   --> tests/by-util/test_env.rs:448:4
    |
448 | fn test_env_with_gnu_reference_parsing_errors() {
    |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: you could split it up into multiple smaller functions
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity
    = note: `-D clippy::cognitive-complexity` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::cognitive_complexity)]`

There are two solutions

* Change the configuration of `cognitive-complexity-threshold` in `.clippy.toml` from `10` to `71`, avoid changing the unit testing function name

* Change from `10` to `24` and rename only one unit testing function name

I think add #[allow(clippy::cognitive_complexity)] would be better because of the clap made a good example.

So I'll add #[allow(clippy::cognitive_complexity)] to resolve the issues.

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Can be merged in my opinion, but I do have a few questions.

@@ -1,2 +1,2 @@
msrv = "1.70.0"
cognitive-complexity-threshold = 10
cognitive-complexity-threshold = 24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait this was super low all this time??? No wonder it was so annoying. Let's just remove this line; the default value is 25 according to the documentation

Copy link
Member

@tertsdiepraam tertsdiepraam Mar 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sylvestre I see that you introduced the lower value. I think 10 is a bit too low, but I'm happy to compromise on something inbetween as well. I think the problem with 10 is that it's a good hint that a function is too complex, but too restrictive to enforce.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, it was to identify too long/complex functions. I am happy where we are now with 25
thanks

tests/by-util/test_env.rs Show resolved Hide resolved
@sylvestre sylvestre merged commit 88ff42e into uutils:main Mar 23, 2024
62 checks passed
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.

3 participants