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

[Merged by Bors] - Remove the dependency cycles #5171

Closed
wants to merge 4 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Jul 1, 2022

Objective

Solution

None of them are remotely necessary, if we import
the trick from bevyengine#2851

This works around rust-lang/rust-analyzer#11410
@DJMcNab DJMcNab marked this pull request as ready for review July 1, 2022 21:18
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Core Common functionality for all bevy apps labels Jul 1, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Clever. I think this is worth doing.

/// App::new().add_plugins(MinimalPlugins).run();
/// }
/// ```
pub struct NoopPluginGroup;
Copy link
Member

Choose a reason for hiding this comment

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

should this be doc hidden?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose so - it's only really useful for dependencies of bevy_internal

@mockersf
Copy link
Member

mockersf commented Jul 1, 2022

I've never had that issue with rust-analyzer, and this could complicate some doc examples. Could you confirm it would fix your issue?

@DJMcNab
Copy link
Member Author

DJMcNab commented Jul 2, 2022

So the issue I've been having has been slightly intermittent

I can confirm that this appeared to resolve it pretty quickly. Without root causing it in rust-analyzer I cannot confirm that this was definitely the reason it was resolved.

@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 Jul 2, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jul 4, 2022
# Objective

- I think our codebase is hit badly by rust-lang/rust-analyzer#11410
- None of our uses of cyclic dependencies are remotely necessary
- Note that these are false positives in rust-analyzer, however it's probably easier for us to work around this
- Note also that I haven't confirmed that this is causing rust-analyzer to not work very well, but it's not a bad guess.

## Solution

- Remove our cyclic dependencies
- Import the trick from #2851 for no-op plugin groups.
@bors bors bot changed the title Remove the dependency cycles [Merged by Bors] - Remove the dependency cycles Jul 4, 2022
@bors bors bot closed this Jul 4, 2022
@DJMcNab DJMcNab deleted the cycles branch July 4, 2022 15:14
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

- I think our codebase is hit badly by rust-lang/rust-analyzer#11410
- None of our uses of cyclic dependencies are remotely necessary
- Note that these are false positives in rust-analyzer, however it's probably easier for us to work around this
- Note also that I haven't confirmed that this is causing rust-analyzer to not work very well, but it's not a bad guess.

## Solution

- Remove our cyclic dependencies
- Import the trick from bevyengine#2851 for no-op plugin groups.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- I think our codebase is hit badly by rust-lang/rust-analyzer#11410
- None of our uses of cyclic dependencies are remotely necessary
- Note that these are false positives in rust-analyzer, however it's probably easier for us to work around this
- Note also that I haven't confirmed that this is causing rust-analyzer to not work very well, but it's not a bad guess.

## Solution

- Remove our cyclic dependencies
- Import the trick from bevyengine#2851 for no-op plugin groups.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- I think our codebase is hit badly by rust-lang/rust-analyzer#11410
- None of our uses of cyclic dependencies are remotely necessary
- Note that these are false positives in rust-analyzer, however it's probably easier for us to work around this
- Note also that I haven't confirmed that this is causing rust-analyzer to not work very well, but it's not a bad guess.

## Solution

- Remove our cyclic dependencies
- Import the trick from bevyengine#2851 for no-op plugin groups.
@BartBM BartBM mentioned this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps 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.

None yet

3 participants