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] - Move get_short_name utility method from bevy_reflect into bevy_utils #5174

Closed
wants to merge 9 commits into from

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Jul 2, 2022

Summary

This method strips a long type name like bevy::render::camera::PerspectiveCameraBundle down into the bare type name (PerspectiveCameraBundle). This is generally useful utility method, needed by #4299 and #5121.

As a result:

  • This method was moved to bevy_utils for easier reuse.
  • The legibility and robustness of this method has been significantly improved.
  • Harder test cases have been added.

This change was split out of #4299 to unblock it and make merging / reviewing the rest of those changes easier.

Changelog

  • added bevy_utils::get_short_name, which strips the path from a type name for convenient display.
  • removed the TypeRegistry::get_short_name method. Use the function in bevy_utils instead.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Utils Utility functions and types C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jul 2, 2022
Copy link
Contributor

@sixfold-origami sixfold-origami left a comment

Choose a reason for hiding this comment

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

One quibble with the docstring. Otherwise, this looks great!

crates/bevy_utils/src/short_names.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@sixfold-origami sixfold-origami left a comment

Choose a reason for hiding this comment

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

Looks good!

tools/spancmp/Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Didn't check line by line, but tests pass, so LGTM.

PS: There are some comment checks.

crates/bevy_utils/src/short_names.rs Outdated Show resolved Hide resolved
crates/bevy_utils/src/short_names.rs Outdated Show resolved Hide resolved
crates/bevy_utils/src/short_names.rs Outdated Show resolved Hide resolved
crates/bevy_utils/src/short_names.rs Outdated Show resolved Hide resolved
@jakobhellermann
Copy link
Contributor

I was wondering whether the new dependency edge from bevy_utils to bevy_reflect would have a negative impact on compilation time. Looking at the output of cargo build --timing, it currently does not, as bevy_reflect is bottlenecked by syn->serde->glam.

image


Alternatively this could go into an entirely different crate. Which is what I needed a while ago for bevy_mod_debugdump so I put the function into a small crate pretty-type-name. We could just use that, unless we want to keep the non-bevy-maintained dependency count low.

Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>
@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 Author

Good to see the compile time shouldn't be affected.

I'd be happy to split apart bevy_utils basically as much as we can; I always dislike catch-all utilities. It's unsurprising that bevy_mod_debugdump needed this too, and further suggests that this needs to be a standalone function. I'd also be in favor of this to make bevy_reflect more self-contained as a general-purpose Rust crate.

That said, creating a different crate for this or pulling in a third-party dependency is significantly more controversial. That should be done in a seperate PR, which should be much easier after this refactoring.

@alice-i-cecile
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Jul 2, 2022
…tils` (#5174)

# Summary

This method strips a long type name like `bevy::render::camera::PerspectiveCameraBundle` down into the bare type name (`PerspectiveCameraBundle`). This is generally useful utility method, needed by #4299 and #5121.

As a result:

- This method was moved to `bevy_utils` for easier reuse.
- The legibility and robustness of this method has been significantly improved.
- Harder test cases have been added.

This change was split out of #4299 to unblock it and make merging / reviewing the rest of those changes easier.

## Changelog

- added `bevy_utils::get_short_name`, which strips the path from a type name for convenient display.
- removed the `TypeRegistry::get_short_name` method. Use the function in `bevy_utils` instead.
@bors bors bot changed the title Move get_short_name utility method from bevy_reflect into bevy_utils [Merged by Bors] - Move get_short_name utility method from bevy_reflect into bevy_utils Jul 2, 2022
@bors bors bot closed this Jul 2, 2022
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
…tils` (bevyengine#5174)

# Summary

This method strips a long type name like `bevy::render::camera::PerspectiveCameraBundle` down into the bare type name (`PerspectiveCameraBundle`). This is generally useful utility method, needed by bevyengine#4299 and bevyengine#5121.

As a result:

- This method was moved to `bevy_utils` for easier reuse.
- The legibility and robustness of this method has been significantly improved.
- Harder test cases have been added.

This change was split out of bevyengine#4299 to unblock it and make merging / reviewing the rest of those changes easier.

## Changelog

- added `bevy_utils::get_short_name`, which strips the path from a type name for convenient display.
- removed the `TypeRegistry::get_short_name` method. Use the function in `bevy_utils` instead.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…tils` (bevyengine#5174)

# Summary

This method strips a long type name like `bevy::render::camera::PerspectiveCameraBundle` down into the bare type name (`PerspectiveCameraBundle`). This is generally useful utility method, needed by bevyengine#4299 and bevyengine#5121.

As a result:

- This method was moved to `bevy_utils` for easier reuse.
- The legibility and robustness of this method has been significantly improved.
- Harder test cases have been added.

This change was split out of bevyengine#4299 to unblock it and make merging / reviewing the rest of those changes easier.

## Changelog

- added `bevy_utils::get_short_name`, which strips the path from a type name for convenient display.
- removed the `TypeRegistry::get_short_name` method. Use the function in `bevy_utils` instead.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…tils` (bevyengine#5174)

# Summary

This method strips a long type name like `bevy::render::camera::PerspectiveCameraBundle` down into the bare type name (`PerspectiveCameraBundle`). This is generally useful utility method, needed by bevyengine#4299 and bevyengine#5121.

As a result:

- This method was moved to `bevy_utils` for easier reuse.
- The legibility and robustness of this method has been significantly improved.
- Harder test cases have been added.

This change was split out of bevyengine#4299 to unblock it and make merging / reviewing the rest of those changes easier.

## Changelog

- added `bevy_utils::get_short_name`, which strips the path from a type name for convenient display.
- removed the `TypeRegistry::get_short_name` method. Use the function in `bevy_utils` instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Utils Utility functions and types C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide 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

5 participants