Skip to content

Conversation

@stevelr
Copy link
Contributor

@stevelr stevelr commented Oct 6, 2023

fix: clippy warnings and missing error return

A couple of these looked like actual bugs, where an anyhow! error was generated but not returned

@stevelr stevelr changed the title Fix/warnings fix: clippy warnings Oct 6, 2023
Signed-off-by: stevelr <[email protected]>
Copy link
Contributor

@abrown abrown left a comment

Choose a reason for hiding this comment

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

@stevelr, thanks for fixing this stuff. Just a couple of nits below...

pub use tensor_desc::TensorDesc;

/// # Panics
///
Copy link
Contributor

@abrown abrown Oct 6, 2023

Choose a reason for hiding this comment

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

Same "explain the panic" comment as below.

.all(|w| w[0].version == w[1].version)
{
anyhow!(
return Err(anyhow!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use bail! here? I haven't been too worried about xtask since it is essentially repo-internal scripts, but thanks for fixing this!

.all(|w| w[0].version == w[1].version)
{
anyhow!(
return Err(anyhow!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same bail! comment as above.

}

/// # Panics
///
Copy link
Contributor

Choose a reason for hiding this comment

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

I think clippy is looking for something like:

/// Find the path...
///
/// # Panics
/// This function panics if it fails to find and load an OpenVINO library.

They want us to describe the conditions under which this panics.

@abrown abrown mentioned this pull request Oct 18, 2023
@abrown
Copy link
Contributor

abrown commented Oct 18, 2023

Thanks, @stevelr! To get this merged sooner I just fixed the suggestions above over in #80 and merged this as a commit co-authored by both of us.

@abrown abrown closed this Oct 18, 2023
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.

2 participants