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

384 add clippy to ci #459

Merged
merged 17 commits into from
Jun 14, 2024
Merged

384 add clippy to ci #459

merged 17 commits into from
Jun 14, 2024

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented May 3, 2024

Closes #384

Implements the same two rules for checking the code with clippy used in Spacewalk, one more strict for the main targets, and one more forgiving for the remaining ones.

Due to the different imports in the package runtime-integration-tests, we need to run a separate checking step only for this package for it to compile properly. The rules are the same as for all targets.

Additionally, some modifications are made to avoid some errors and warnings now that the check is implemented.

@gianfra-t gianfra-t linked an issue May 3, 2024 that may be closed by this pull request
@gianfra-t gianfra-t requested a review from a team May 6, 2024 13:02
@gianfra-t gianfra-t marked this pull request as ready for review May 6, 2024 13:04
Copy link
Member

@ebma ebma 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 to me 👍 Let's try to fix all the warnings that are now shown in this PR though.

@gianfra-t
Copy link
Contributor Author

Yes there are many warnings about unused functions that I believe happen only because of how clippy is analyzing the crates in an isolated manner. I will look for a flag to skip that warnings.

On the other hand, there are some warnings (that I rolled back before) because I thought it was modifying a very critical part of the chain extension in this case which is hard to test properly. I will push that again and we can discuss it.

@gianfra-t gianfra-t requested a review from ebma May 8, 2024 19:13
pallets/parachain-staking/src/types.rs Outdated Show resolved Hide resolved
chain-extensions/token/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@ebma ebma 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 to me, I think we can merge once the CI passed 👍

@gianfra-t
Copy link
Contributor Author

We can merge this after fixes that will come shortly here for compiling with --all-features.

runtime/amplitude/src/lib.rs Outdated Show resolved Hide resolved
@ebma ebma changed the title 384 add clippy to ci [On hold] 384 add clippy to ci May 24, 2024
Copy link
Contributor

@bogdanS98 bogdanS98 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 to me too! 👍🏼 @gianfra-t #178 is merged now FYI.

@bogdanS98 bogdanS98 self-requested a review June 12, 2024 08:22
@gianfra-t
Copy link
Contributor Author

I will try to remove some of the warnings and then merge!

@gianfra-t gianfra-t changed the title [On hold] 384 add clippy to ci 384 add clippy to ci Jun 13, 2024
@gianfra-t
Copy link
Contributor Author

gianfra-t commented Jun 13, 2024

@pendulum-chain/devs I was not able to remove the warning for impl X::Config for Runtime {}, I don't think it's critical to remove it, so I would just merge this and later we can explore how to refactor so it doesn't warn.

@gianfra-t gianfra-t requested a review from bogdanS98 June 13, 2024 19:54
@ebma
Copy link
Member

ebma commented Jun 14, 2024

Can we just disable the 'non_local_definitions' rule for the clippy checks then? I remember I was also struggling with this in Spacewalk, IIRC @b-yap and I just changed back the Rust toolchain version to an older one but that's of course not ideal.
Also, let's change the CI so that warnings get denied with this flag. Otherwise the clippy checks are easily missed/ignored. If we deny every warning and ignore the 'non_local_definitions' error, we need to fix the three other minor issues shown (use of constant weight and unused imports).

@gianfra-t
Copy link
Contributor Author

@ebma thanks for the linting reference, I thought about ignoring the local impl but couldn't find the lint name to allow.

I also replaced the use of the direct definition of the weight of pallet vesting here. It is still manual, but at least clippy will not complain. Otherwise we need to do a benchmark just for this extrinsic.

Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Great 👍 looks good to me

@gianfra-t gianfra-t merged commit 47b0666 into main Jun 14, 2024
2 checks passed
@gianfra-t gianfra-t deleted the 384-add-clippy-to-ci branch June 14, 2024 21:07
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.

Add clippy to CI
4 participants