pendulum icon indicating copy to clipboard operation
pendulum copied to clipboard

384 add clippy to ci

Open gianfra-t opened this issue 1 year ago • 2 comments

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 avatar May 03 '24 16:05 gianfra-t

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 avatar May 06 '24 16:05 gianfra-t

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

gianfra-t avatar May 14 '24 18:05 gianfra-t

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

gianfra-t avatar Jun 12 '24 12:06 gianfra-t

@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 avatar Jun 13 '24 19:06 gianfra-t

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).

ebma avatar Jun 14 '24 08:06 ebma

@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.

gianfra-t avatar Jun 14 '24 12:06 gianfra-t