search

pendulum-chain / pendulum

GNU General Public License v3.0
43 stars 14 forks source link

384 add clippy to ci #459

Closed gianfra-t closed 5 months ago

gianfra-t commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

gianfra-t commented 5 months ago

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

gianfra-t commented 5 months ago

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

ebma commented 5 months ago

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 commented 5 months ago

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