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

Remove redundant imports #12817

Merged
merged 6 commits into from Apr 1, 2024
Merged

Remove redundant imports #12817

merged 6 commits into from Apr 1, 2024

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Mar 31, 2024

Objective

  • There are several redundant imports in the tests and examples that are not caught by CI because additional flags need to be passed.

Solution

  • Run cargo check --workspace --tests and cargo check --workspace --examples, then fix all warnings.
  • Add test-check to CI, which will be run in the check-compiles job. This should catch future warnings for tests. Examples are already checked, but I'm not yet sure why they weren't caught.

Discussion

Found using `cargo check --workspace --tests`.
Found using `cargo check --workspace --examples`.
@BD103 BD103 changed the title Redundant imports Remove redundant imports Mar 31, 2024
@BD103 BD103 added C-Examples An addition or correction to our examples C-Code-Quality A section of code that is hard to understand or change A-Core Common functionality for all bevy apps C-Testing A change that impacts how we test Bevy or how users test their apps labels Mar 31, 2024
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably extend CI to do these checks with these extra flags.

@james7132 james7132 added A-Cross-Cutting Impacts the entire engine and removed A-Core Common functionality for all bevy apps labels Apr 1, 2024
@BD103 BD103 marked this pull request as draft April 1, 2024 00:19
github-merge-queue bot pushed a commit that referenced this pull request Apr 1, 2024
# Objective

- The `ImePreedit` component from
[`text_input`](https://github.com/bevyengine/bevy/blob/50699ecf76800e5b1b31bd9b6845b30e4ede5e32/examples/input/text_input.rs#L126-L127)
appears to be unused.
- This was found by running `cargo check --workspace --examples`,
originally as part of #12817.

## Solution

- Remove it :)
@pablo-lua pablo-lua added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Apr 1, 2024
@pablo-lua
Copy link
Contributor

Removed the label because it's a draft

@BD103
Copy link
Member Author

BD103 commented Apr 1, 2024

Removed the label because it's a draft

Yup, I just need to update the CI and I'll mark it as ready.

@BD103 BD103 marked this pull request as ready for review April 1, 2024 15:12
@pablo-lua pablo-lua 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 Apr 1, 2024
@BD103
Copy link
Member Author

BD103 commented Apr 1, 2024

I added a check into CI, but it's not amazing. I think the tool needs a refactoring, but for now this PR is ready to go.

tools/ci/src/main.rs Outdated Show resolved Hide resolved
@mockersf mockersf removed 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 Apr 1, 2024
Co-authored-by: François Mockers <francois.mockers@vleue.com>
@mockersf mockersf 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 Apr 1, 2024
@mockersf mockersf added this pull request to the merge queue Apr 1, 2024
Merged via the queue into bevyengine:main with commit 84363f2 Apr 1, 2024
28 checks passed
@BD103 BD103 deleted the redundant-imports branch April 1, 2024 21:15
chompaa pushed a commit to chompaa/bevy that referenced this pull request Apr 5, 2024
…ngine#12818)

# Objective

- The `ImePreedit` component from
[`text_input`](https://github.com/bevyengine/bevy/blob/50699ecf76800e5b1b31bd9b6845b30e4ede5e32/examples/input/text_input.rs#L126-L127)
appears to be unused.
- This was found by running `cargo check --workspace --examples`,
originally as part of bevyengine#12817.

## Solution

- Remove it :)
chompaa pushed a commit to chompaa/bevy that referenced this pull request Apr 5, 2024
# Objective

- There are several redundant imports in the tests and examples that are
not caught by CI because additional flags need to be passed.

## Solution

- Run `cargo check --workspace --tests` and `cargo check --workspace
--examples`, then fix all warnings.
- Add `test-check` to CI, which will be run in the check-compiles job.
This should catch future warnings for tests. Examples are already
checked, but I'm not yet sure why they weren't caught.

## Discussion

- Should the `--tests` and `--examples` flags be added to CI, so this is
caught in the future?
- If so, bevyengine#12818 will need to be merged first. It was also a warning
raised by checking the examples, but I chose to split off into a
separate PR.

---------

Co-authored-by: François Mockers <francois.mockers@vleue.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine 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.

None yet

6 participants