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

Feature gate use_shortnames #15348

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Sep 21, 2024

Objective

  • use_shortnames only does anything if bevy_reflect feature is enabled, so it makes sense to put it behind a feature.

Solution

  • #[cfg] on the if block and the field declaration

Testing

  • CI

Migration Guide

  • If you've disabled bevy_reflect feature of bevy_ecs, remove references to that field.

Context

See: #15340

@BenjaminBrienen
Copy link
Contributor Author

Might want to merge #15340 first, as this is branched from there

@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Sep 21, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it A-Reflection Runtime information about types labels Sep 21, 2024
@hymm
Copy link
Contributor

hymm commented Sep 21, 2024

Send weird to me that short names in the schedule would be feature gated to bevy reflect. As it doesn't have anything to do with that feature. Feels more like an argument against #15340

@BenjaminBrienen
Copy link
Contributor Author

Send weird to me that short names in the schedule would be feature gated to bevy reflect. As it doesn't have anything to do with that feature. Feels more like an argument against #15340

#15340 doesn't change any functionality. It just moves the existing code from bevy_utils into bevy_reflect.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 21, 2024
@BenjaminBrienen
Copy link
Contributor Author

I'm just gonna merge this into the other branch, actually.

@BenjaminBrienen BenjaminBrienen deleted the feature-gate-use_shortnames branch September 21, 2024 20:33
@hymm
Copy link
Contributor

hymm commented Sep 21, 2024

The fact that you had to feature gate use_short_name for schedules says to me that there was a change in functionality.

@BenjaminBrienen
Copy link
Contributor Author

The fact that you had to feature gate use_short_name for schedules says to me that there was a change in functionality.

I didn't have to. That was an enhancement on top of the simple refactor.

@hymm
Copy link
Contributor

hymm commented Sep 21, 2024

So the use_shortnames flag still worked without the bevy_reflect feature enabled? I find that hard to believe.

@BenjaminBrienen
Copy link
Contributor Author

So the use_shortnames flag still worked without the bevy_reflect feature enabled? I find that hard to believe.

Sorry, you're right. I was drawing a distinction between "have to feature gate use_shortname" and "have to feature gate the place where its used". I guess the PR can be reverted if the maintainers decide this is unwanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events 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.

3 participants