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] - Use bevy_reflect as path in case of no direct references #1875

Closed
wants to merge 11 commits into from

Conversation

YohDeadfall
Copy link
Member

@YohDeadfall YohDeadfall commented Apr 10, 2021

Fixes #1844

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Core Common functionality for all bevy apps labels Apr 10, 2021
@mtsr

This comment has been minimized.

@YohDeadfall
Copy link
Member Author

YohDeadfall commented Apr 12, 2021

Did a typo which while writing the commit message, but quickly fixed it.

@alice-i-cecile alice-i-cecile added the A-Reflection Runtime information about types label Apr 14, 2021
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.

This seems like a sensible approach.

@YohDeadfall
Copy link
Member Author

@alice-i-cecile, shouldn't all approved PR get the ready for @cart tag?

@alice-i-cecile
Copy link
Member

@alice-i-cecile, shouldn't all approved PR get the ready for @cart tag?

Standard rule of thumb is two community reviews approving it <3 This only has one still, and it's not so trivial that I think it's correct to bend that.

@mockersf
Copy link
Member

There are other places where we use find-crate (bevy_derive & bevy_ecs/macros). Would it make sense to unify them to cargo-manifest?

@YohDeadfall
Copy link
Member Author

I thought to make a follow up PR to cleanup other places to and unify the logic. It's more like a refactoring, so not included all changes here. This one for fixing the issue, another introduces a new crate and moves all path getting logic to it.

Add other changes here too?

@alice-i-cecile
Copy link
Member

I would prefer it as one PR; it'll be easier to review that way and we don't risk forgetting.

@cart
Copy link
Member

cart commented Apr 25, 2021

Add other changes here too?

Yeah I think that makes the most sense. Just do it in a separate commit so we can revert it if necessary.

@YohDeadfall
Copy link
Member Author

Not a problem, but you have to manually merge it then since bors squashes them as I understand to avoid noise like fixups.

@cart
Copy link
Member

cart commented Apr 25, 2021

Not a problem, but you have to manually merge it then since bors squashes them as I understand to avoid noise like fixups.

Sorry I wasn't clear enough. I meant "revent" in the context of "pre-merged pr". Once we merge I don't think we'll need that level of granularity.

@cart
Copy link
Member

cart commented Apr 27, 2021

Semi-lazy question: what motivated the move from find-crate to cargo-manifest?

@YohDeadfall
Copy link
Member Author

A bit better API on dependency handling. find-crate requires setting type of dependencies on the manifest to use it's find methods, while cargo-manifest represents a manifest as is, so there are two members for dependencies: dependencies and dev_dependencies.

Copy link
Contributor

@MinerSebas MinerSebas left a comment

Choose a reason for hiding this comment

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

Please also add the new bevy_macro_utils crate to the publish.sh File in the tools Folder.

@YohDeadfall
Copy link
Member Author

@MinerSebas, added, but looking on how crates depends on each other it might make sense to move the module resolution logic to bevy_utils. It shouldn't increase compilation time.

crates/bevy_macro_utils/Cargo.toml Show resolved Hide 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 Apr 29, 2021
@cart
Copy link
Member

cart commented May 18, 2021

Just refactored Manifest logic into BevyManifest. This accomplishes a few things:

  1. Removes the need to import floating helper functions
  2. Makes it easier to reuse Manifest import work
  3. Enabled the removal of Modules from bevy_derive. Modules required extracting all bevy module info from Manifest instead of just whats needed. The new BevyManifest::get_path(BEVY_RENDER) pattern allows us to only parse the manifest once and only extract the information we need.

I also removed the as_crate attribute. I don't think it was worth the extra complexity and it had user-facing performance implications for solving a problem that only really exists in internal bevy crates. The use crate as bevy_X pattern works fine for using derive macros in the crates they are defined.

@cart
Copy link
Member

cart commented May 19, 2021

bors r+

bors bot pushed a commit that referenced this pull request May 19, 2021
Fixes #1844


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors bors bot changed the title Use bevy_reflect as path in case of no direct references [Merged by Bors] - Use bevy_reflect as path in case of no direct references May 19, 2021
@bors bors bot closed this May 19, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
@YohDeadfall YohDeadfall deleted the reflect-module-paths branch October 26, 2021 08:32
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 A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior 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.

Unable to use bevy_reflect derive macros when re-exported
7 participants