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

core/identity/ecdsa: fix clippy::op-ref, clippy::needless-borrow #2770

Merged
merged 2 commits into from
Jul 25, 2022

Conversation

elenaf9
Copy link
Contributor

@elenaf9 elenaf9 commented Jul 24, 2022

Description

Fix clippy::op-ref, clippy::needless-borrow, which cause an error if you run cargo custom-clippy directly in the /core package or depend on it. See https://github.com/kpp/rust-libp2p/runs/7489162901?check_suite_focus=true.

Does this need a changelog entry?

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

No changelog entry needed IMO :)

But curious why this isn't flagged by CI?

@elenaf9
Copy link
Contributor Author

elenaf9 commented Jul 24, 2022

But curious why this isn't flagged by CI?

I was also confused. The same happened in #2737. Took me actually a while to figure it out, though in hindsight the answer is rather obvious:
Our custom-clippy just runs clippy in the root package with default features, and in our root Cargo.toml we disable default features for dependencies like libp2p-core. Since ecdsa is behind a features flag it was not checked.
I think it would make more sense to check all packages with all features. Opened #2771.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and thanks for looking into why this wasn't caught before.

Also don't think this needs a changelog.

@mxinden mxinden merged commit ce963df into libp2p:master Jul 25, 2022
@elenaf9 elenaf9 deleted the core/identity-fix-clippy branch July 25, 2022 16:02
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