Clippy practices

[adizere]

Crate

ibc

Summary

We have inconsistent practices regarding the use of clippy exceptions, in particular to too_many_arguments and large_enum_variant. We should decide on a standard.

Problem Definition

We sometimes annotate enums with #[allow(clippy::large_enum_variant)] to avoid clippy warnings:

https://github.com/informalsystems/ibc-rs/blob/12cb1d6187e878eee0e40295fe0b804ddef0b846/modules/src/ics02_client/msgs.rs#L13-L18

And sometimes we box large enum variants for the same reason (to avoid clippy warnings):

https://github.com/informalsystems/ibc-rs/blob/12cb1d6187e878eee0e40295fe0b804ddef0b846/modules/src/ics03_connection/msgs.rs#L29-L34

The too_many_arguments check also poses issues, for example this method has 11 arguments, but clippy issues warnings for anything larger than 7:

https://github.com/informalsystems/ibc-rs/blob/12cb1d6187e878eee0e40295fe0b804ddef0b846/modules/src/ics07_tendermint/client_state.rs#L34-L47

While we're at it, we should also consider doing a sweeping through the repo and clean other clippy exception, for example:

https://github.com/informalsystems/ibc-rs/blob/c32393a4f344877e34957d13a768e6623486c587/modules/src/tx_msg.rs#L15

Improvement proposal:

• perhaps we can eliminate methods that have excessive number of arguments
• use a repo-wide clippy configuration with larger thresholds than the defaults

Original discussion

I wonder if we can have a clippy config to parametrize the "too_many_arguments" threshold (default is 7) for this crate, since we're using this allow quite often. We are also cutting some corners with allowing large_enum_variant.

Originally posted by @adizere in #355 (comment)

In future, I think this deserves a discussion about clippy in general, if we want to have a repo-wide configuration where we adjust the clippy parameters ... so we can remove the allow/clippy annotations altogether. I can open an issue to track this if that seems interesting.

Originally posted by @adizere in #355 (comment)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[mzabaluev]

I would strongly advocate not to override too_many_arguments. The example above is impossible to use without looking at the definition or using extra aids in the IDE. The call sites may be difficult to understand without IDE-provided hints or comments (what each of the three Duration arguments mean? There are two boolean flags as well, with another antipattern mixed in).

Instead, we should refactor the code to use:

• struct parameters, to benefit from named field initializers and Default;
• the builder pattern for constructing complex values, especially when many of the fields can be initialized with default values.

[ancazamfir]

Closing this, we can open new ones with concrete proposal and following up with PRs.