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

Update lints used in Twilight #2350

Open
laralove143 opened this issue Jun 3, 2024 · 0 comments
Open

Update lints used in Twilight #2350

laralove143 opened this issue Jun 3, 2024 · 0 comments
Labels
c-all Affects all crates or the project as a whole t-refactor Refactors APIs or code.

Comments

@laralove143
Copy link
Member

There are many lints in Rustc and Clippy that fall in line with our contribution guides that we are not making use of. Our way of setting lints could also be improved.

References for new lints

Rustc lints

A list of opt-in Rustc lints can be found here. It is best to only opt out of default lints on a statement-basis, rather than a workspace/crate basis.

Clippy Lints

These lint groups are opt-in and most lints in them are applicable for our use-case:

Lints in the group of Restriction should not be enabled as a whole but there are some lints that we might find useful:

Some of these lints might be noisy, some of them might require a significant number of changes to apply, some may be unnecessary for our purposes, and some may actually not follow our current code-style. They are listed here as a reference for further discussion before a PR.

Some lints that should be here could be missing out of my mistake. The lints are ordered alphabetically (as they appear on the Clippy lints website) and comments to add missing lints are welcome.

New Way of Setting Lints

Currently, we are setting lints in each crate's respective lib.rs/main.rs file, which can cause discrepancies and leads to duplicate code across each crate. We were doing this because there was no other way to set lints. We can now set lints in Cargo.toml files (documentation, tracking issue). A few questions remain on whether this approach is viable though:

  • Is there an MSRV implication of using this? Will this require us to increase our MSRV? Are the advantages of this usage worth the disadvantage of increasing our MSRV?
  • Is this allowed in workspaces? Can we set lints for all crates in our workspace in the top-level Cargo.toml? If not, we would still have to duplicate the lints in all Cargo.toml files, which removes the advantage of this approach.

I was unable to find the answers to these questions but we can find out by simply trying.

@laralove143 laralove143 added c-all Affects all crates or the project as a whole t-refactor Refactors APIs or code. labels Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-all Affects all crates or the project as a whole t-refactor Refactors APIs or code.
Projects
None yet
Development

No branches or pull requests

1 participant