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

Allow clippy::type_complexity in more places. #9796

Merged

Conversation

waywardmonkeys
Copy link
Contributor

Objective

  • See fewer warnings when running cargo clippy locally.

Solution

  • allow clippy::type_complexity in more places, which also signals to users they should do the same.

@IceSentry
Copy link
Contributor

When running clippy in CI we use pre configured lints because we don't want to pollute the repo with a bunch of annotations like that.

Personally, I added this config to my rust analyzer to use the same lints as bevy

   "rust-analyzer.check.command": "clippy",
   "rust-analyzer.check.extraArgs": [
      "--all-features",
      "--",
      "-Aclippy::type_complexity",
      "-Wclippy::doc_markdown",
      "-Wclippy::redundant_else",
      "-Wclippy::match_same_arms",
      "-Wclippy::semicolon_if_nothing_returned",
      "-Wclippy::explicit_iter_loop",
      "-Wclippy::map_flatten",
      "-Dwarnings"
  ],

@waywardmonkeys
Copy link
Contributor Author

I thought about that, but there were already about 37 of these in the code (every crate basically except the new asset crate).

@alice-i-cecile alice-i-cecile added A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change C-Testing A change that impacts how we test Bevy or how users test their apps labels Sep 13, 2023
@JoJoJet
Copy link
Member

JoJoJet commented Sep 14, 2023

We should do this -- see #8313 which did this for everything except for our examples. Initially I didn't do this for the examples because it just seemed like a lot, but given that disabling this lint is considered best practice, it's a good idea to teach users via our examples.

We should add a comment to each one, something like

// This lint usually gives bad advice in the context of Bevy -- hiding complex queries behind
// type alias tends to obfuscate code while offering no improvement in code cleanliness.

I'd go a step further and remove the lint suppression from CI, so we have only one source of truth for this lint (which was the opinion cart originally expressed in the aforementioned PR).

@waywardmonkeys
Copy link
Contributor Author

I'll update this. To avoid a conflict, we'll need #9794 to land before this.

@waywardmonkeys waywardmonkeys force-pushed the allow-type-complexity branch 4 times, most recently from 807faf7 to 2cf0100 Compare September 14, 2023 04:42
@waywardmonkeys
Copy link
Contributor Author

I removed -A clippy::type_complexity from CI as discussed above and elsewhere. If examples change in such a way that they start to trigger, then they would need to be updated since this PR doesn't edit every single example, only those that currently trigger the lint.

@JoJoJet JoJoJet added C-Docs An addition or correction to our documentation C-Examples An addition or correction to our examples and removed C-Docs An addition or correction to our documentation labels Sep 14, 2023
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 21, 2023
@alice-i-cecile
Copy link
Member

@waywardmonkeys can you fix merge conflicts and then I'll merge?

@waywardmonkeys
Copy link
Contributor Author

Sure thing. I am almost done with dinner and then will do it

@waywardmonkeys
Copy link
Contributor Author

@alice-i-cecile Done! Hopefully everything is correct with the latest push.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 2, 2023
Merged via the queue into bevyengine:main with commit 9a798aa Oct 2, 2023
21 checks passed
@waywardmonkeys waywardmonkeys deleted the allow-type-complexity branch October 2, 2023 23:24
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
# Objective

- See fewer warnings when running `cargo clippy` locally.

## Solution

- allow `clippy::type_complexity` in more places, which also signals to
users they should do the same.
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
# Objective

- See fewer warnings when running `cargo clippy` locally.

## Solution

- allow `clippy::type_complexity` in more places, which also signals to
users they should do the same.
github-merge-queue bot pushed a commit that referenced this pull request Nov 18, 2023
…erywhere (#10011)

# Objective

- Fix adding `#![allow(clippy::type_complexity)]` everywhere. like #9796

## Solution

- Use the new [lints] table that will land in 1.74
(https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints)
- inherit lint to the workspace, crates and examples.
```
[lints]
workspace = true
```

## Changelog

- Bump rust version to 1.74
- Enable lints table for the workspace
```toml
[workspace.lints.clippy]
type_complexity = "allow"
```
- Allow type complexity for all crates and examples
```toml
[lints]
workspace = true
```

---------

Co-authored-by: Martín Maita <47983254+mnmaita@users.noreply.github.com>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- See fewer warnings when running `cargo clippy` locally.

## Solution

- allow `clippy::type_complexity` in more places, which also signals to
users they should do the same.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…erywhere (bevyengine#10011)

# Objective

- Fix adding `#![allow(clippy::type_complexity)]` everywhere. like bevyengine#9796

## Solution

- Use the new [lints] table that will land in 1.74
(https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#lints)
- inherit lint to the workspace, crates and examples.
```
[lints]
workspace = true
```

## Changelog

- Bump rust version to 1.74
- Enable lints table for the workspace
```toml
[workspace.lints.clippy]
type_complexity = "allow"
```
- Allow type complexity for all crates and examples
```toml
[lints]
workspace = true
```

---------

Co-authored-by: Martín Maita <47983254+mnmaita@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples C-Testing A change that impacts how we test Bevy or how users test their apps S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants