Skip to content

Check for bevy_internal imports in CI#9612

Merged
cart merged 5 commits intobevyengine:mainfrom
ameknite:ci-bevy_internal-imports
Aug 29, 2023
Merged

Check for bevy_internal imports in CI#9612
cart merged 5 commits intobevyengine:mainfrom
ameknite:ci-bevy_internal-imports

Conversation

@ameknite
Copy link
Copy Markdown
Contributor

Objective

  • Avoid using bevy_internal imports in examples.

Solution

I don't know much about CI so I don't know if this is the better approach, but I think is better than doing a pull request every time I found this lol, any suggestion is welcome.

@rparrett
Copy link
Copy Markdown
Contributor

Cool. Found a couple minor issues and suggested fixes.

internal

@rparrett rparrett 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 labels Aug 29, 2023
ameknite and others added 2 commits August 29, 2023 00:18
Co-authored-by: Rob Parrett <[email protected]>
Co-authored-by: Rob Parrett <[email protected]>
@mockersf
Copy link
Copy Markdown
Member

@cart what do you think of adding it to the required checks?

@alice-i-cecile alice-i-cecile 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 Aug 29, 2023
@alice-i-cecile
Copy link
Copy Markdown
Member

IMO this should be in the required checks: it's quick and should be quite robust to both false positives and negatives.

@ameknite we may also want to scan the tests folder too, since the same problem could arise there :)

Copy link
Copy Markdown
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Definitely on board for this (and for making it a required check). I've noticed that Rust Analyzer recommends bevy_internal more regularly now for bevy examples. I'll add this to the required checks after merging (required checks are a repo setting)

@cart cart added this pull request to the merge queue Aug 29, 2023
Merged via the queue into bevyengine:main with commit f2f39c8 Aug 29, 2023
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Avoid using bevy_internal imports in examples. 

## Solution

- Add CI to check for bevy_internal imports like suggested in
bevyengine#9547 (comment)
- Fix another import

I don't know much about CI so I don't know if this is the better
approach, but I think is better than doing a pull request every time I
found this lol, any suggestion is welcome.

---------

Co-authored-by: Rob Parrett <[email protected]>
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 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