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

move ShortName to bevy_reflect #15340

Merged
merged 17 commits into from
Sep 21, 2024

Conversation

BenjaminBrienen
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen commented Sep 20, 2024

Objective

Solution

  • Move the file short_name wholesale into bevy_reflect

Testing

  • Unit tests
  • CI

Migration Guide

  • References to bevy_utils::ShortName should instead now be bevy_reflect::ShortName.

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.

Oh good, I was worried this would mess up the feature flags for bevy_ecs. Nice to see this work cleanly.

I think this makes sense there: it's a tool to work with type names.

@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide A-Reflection Runtime information about types A-Utils Utility functions and types 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 labels Sep 20, 2024
@alice-i-cecile
Copy link
Member

Can you update your migration guide to be a bit more descriptive? It doesn't actually describe the fix when read outside of the context of this PR.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged 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 Sep 20, 2024
BenjaminBrienen and others added 2 commits September 21, 2024 02:05
Co-authored-by: François Mockers <francois.mockers@vleue.com>
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 21, 2024
crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 21, 2024
@alice-i-cecile alice-i-cecile 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 21, 2024
@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
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 21, 2024
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Sep 21, 2024
@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
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 21, 2024
Merged via the queue into bevyengine:main with commit 02a9ed4 Sep 21, 2024
30 checks passed
@hymm
Copy link
Contributor

hymm commented Sep 21, 2024

Oh good, I was worried this would mess up the feature flags for bevy_ecs. Nice to see this work cleanly.

Given that use_shortnames for schedules needed to be feature flagged behind bevy reflect, I'm not sure that this is a clean change. That feature is about using shortnames when reporting ambiguities and has nothing to do with bevy reflect.

@BenjaminBrienen BenjaminBrienen deleted the move-get_short_name branch September 21, 2024 21:49
@BenjaminBrienen
Copy link
Contributor Author

Oh good, I was worried this would mess up the feature flags for bevy_ecs. Nice to see this work cleanly.

Given that use_shortnames for schedules needed to be feature flagged behind bevy reflect, I'm not sure that this is a clean change. That feature is about using shortnames when reporting ambiguities and has nothing to do with bevy reflect.

I think I see what you mean now. What do you suggest as a solution, keeping in mind we, ideally, don't want a utils crate.

@hymm
Copy link
Contributor

hymm commented Sep 21, 2024

I think the only real options are to move it into it's own crate in the bevy repo or move it out like we did with atomicow.

@BenjaminBrienen BenjaminBrienen 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 Nov 10, 2024
@BenjaminBrienen BenjaminBrienen self-assigned this Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types A-Utils Utility functions and types C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants