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 all clippy errors and update versions #442

Merged
merged 3 commits into from Jan 27, 2022

Conversation

Ragnyll
Copy link
Contributor

@Ragnyll Ragnyll commented Jan 23, 2022

What this PR does / why we need it:
closes #383
This fixes all current clippy lint errors and adds ci checks for clippy.

Special notes for your reviewer:
#383 mentions "adding a clippy.toml file" for addressing function has too many arguments but i elected not to add that in interest of #[allow(clippy::too_many_arguments)] instead. This makes those functions with too many args the exception rather than a rule. If these need to be refactored into some kind of struct later than it may be worth logging an issue specifically for that.

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • version has been updated appropriately (./version.sh)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

Signed-off-by: ragnyll <ragnyll@gallowzhumour.dev>
@@ -140,8 +140,8 @@ impl DevicePluginBuilderInterface for DevicePluginBuilder {
UnixListener::bind(task_socket_path).expect("Failed to bind to socket path");

async_stream::stream! {
while let item = uds.accept().map_ok(|(st, _)| unix_stream::UnixStream(st)).await {
Copy link
Contributor Author

@Ragnyll Ragnyll Jan 23, 2022

Choose a reason for hiding this comment

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

i was a little confused handling these. The way this is written it looks like the while loop is trying to break when there's no more connections to accept ( i think). but according to clippy this is an irrefutable statement that never breaks.

warning: irrefutable `while let` pattern
   --> discovery-utils/src/discovery/mod.rs:231:27
    |
231 |                     while let item = uds.accept().map_ok(|(st, _)| unix_stream::UnixStream(st)).await {
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(irrefutable_let_patterns)]` on by default
    = note: this pattern will always match, so the loop will never exit
    = help: consider instead using a `loop { ... }` with a `let` inside it

but matching this and yielding the unix stream doesnt work because this fn returns a Result.

So ultimately i am a little concerned if this is the desired functionality. I trusted clippy and thought id make a note of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i dont think this causes an infinite loop though. Otherwise any test that was using the test version of this would have failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This loop looks fine -- it almost matches the tonic example. The item from the stream could potentially be stored in an item variable like in the example for better readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, i see what you mean by the tonic example too.
refactored all these to be

let item = ...
yield item;

in f333e78

@kate-goldenring
Copy link
Contributor

@Ragnyll, thanks for picking this up! You're approach of using #[allow(clippy::too_many_arguments)] flags instead of a clippy.toml file is super sound -- it also makes it clear exactly which functions need to be updated in the future. I'll take a look at your comments tomorrow but this is looking great.

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Just one nit to avoid the needless_collect flag and this looks good to go. Thanks for all this work!

@@ -894,6 +894,7 @@ mod device_plugin_service_tests {
.collect();
assert_eq!(devices.len(), capacity);
// Can't use map on Device type
#[allow(clippy::needless_collect)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be optimal to refactor instead of adding flags where possible/not extensive.
I believe the following will work without the flag:

        let mut device_ids = devices.into_iter().map(|device| device.id);
        for device in expected_device_ids {
            assert!(device_ids.any(|d| d == device));
        }

Copy link
Contributor Author

@Ragnyll Ragnyll Jan 26, 2022

Choose a reason for hiding this comment

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

sorry @kate-goldenring I dont think gonna work. The any consumes the iterator. Here's an example if you wanna see what I mean: gist (this made me think i was going crazy for a sec when i was trying those). So if the two sets of devices are not in the same order it'll eat up the non matching ones before it starts matching.

anyways reconstructing the iterator for each any in the loop works, and clippy doesnt complain: 8f48d04

if you dont like that i'd say just go back to the version with the allow needless collect, this new way is kinda weird, so i'm not sure which is better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really cool investigation -- this could be a blog post if not reason to put up a bug against the allow needless collect flag on clippy. Like you said, using the flag is neater more efficient code but what you have now is fine.

Signed-off-by: ragnyll <ragnyll@gallowzhumour.dev>
Signed-off-by: ragnyll <ragnyll@gallowzhumour.dev>
Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Once all checks pass, we can merge!

@kate-goldenring kate-goldenring merged commit 4cad8e7 into project-akri:main Jan 27, 2022
@kate-goldenring kate-goldenring mentioned this pull request Jan 27, 2022
8 tasks
@Ragnyll
Copy link
Contributor Author

Ragnyll commented Jan 27, 2022

Huzzah! It's merged. Thanks @kate-goldenring . I'm taking a look around clippy to see if it's worth logging an issue for needless collect thing. There's a ton for that lint already that may fit, but I'll see.

Thanks again!

@Ragnyll Ragnyll deleted the fix_clippy_warnings branch February 8, 2022 06:54
leoluKL pushed a commit to leoluKL/akri that referenced this pull request Sep 23, 2022
Signed-off-by: Leo Lu <leolu@microsoft.com>
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.

Fix cargo clippy warnings
2 participants