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 warnings and add clippy to the CI #455

Merged
merged 21 commits into from
Jun 19, 2023

Conversation

koute
Copy link
Contributor

@koute koute commented Jun 16, 2023

Clippy doesn't like the theoretically redundant closure calls that I've added in my last PR, and since clippy doesn't run on the CI here I didn't notice.

While I was at it I also fixed all other clippy warnings, plus one error as it was complaining about an unsafe that I've now removed. I've also added clippy to the CI (hopefully it works).

Fixes #454

Fixes #456

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Maybe we should add a Clippy config file?
Could be the one from Substrate in .cargo/config.toml.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Looks good, just some Qs.

CHANGELOG.md Outdated Show resolved Hide resolved
src/codec.rs Show resolved Hide resolved
src/compact.rs Outdated Show resolved Hide resolved
src/encode_append.rs Outdated Show resolved Hide resolved
src/encode_append.rs Outdated Show resolved Hide resolved
@koute
Copy link
Contributor Author

koute commented Jun 16, 2023

Maybe we should add a Clippy config file? Could be the one from Substrate in .cargo/config.toml.

Hmmm.... I think we should be fine with the default one here, as this is orders of magnitude smaller so we don't need to disable a bunch of lints like in Substrate.

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Copy link
Member

ggwpez commented Jun 16, 2023

Hmmm.... I think we should be fine with the default one here, as this is orders of magnitude smaller so we don't need to disable a bunch of lints like in Substrate.

Its not just disabling, it is also denying some. But only correctness and complexity as it seems.
Up to you 🤷 Just denying those is probably enough.

src/codec.rs Show resolved Hide resolved
src/encode_append.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@koute
Copy link
Contributor Author

koute commented Jun 19, 2023

Added a fix + test for #456 and also fixed clippy lints in the derive crate (this was as far as I can see necessary to test the fix for #456). Should be good to go once the CI passes.

tests/clippy.rs Show resolved Hide resolved
.gitlab-ci.yml Show resolved Hide resolved
@koute koute merged commit 6a81c42 into paritytech:master Jun 19, 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
4 participants