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

Remove TypeUuid #11497

Merged
merged 7 commits into from Jan 25, 2024
Merged

Remove TypeUuid #11497

merged 7 commits into from Jan 25, 2024

Conversation

AxiomaticSemantics
Copy link
Contributor

@AxiomaticSemantics AxiomaticSemantics commented Jan 23, 2024

Objective

TypeUuid is deprecated, remove it.

Migration Guide

Convert any uses of #[derive(TypeUuid)] with #[derive(TypePath] for more complex uses see the relevant documentation for more information.

@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Jan 23, 2024
@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change labels Jan 23, 2024
@AxiomaticSemantics
Copy link
Contributor Author

AxiomaticSemantics commented Jan 23, 2024

I have ran into one issue with the tests failing due to import conflicts and attempted to use #[rustfmt=skip] to prefix serde import with as ::serde to no avail. resolved

I may have been a bit overzealous adding a second #[rustfmt=skip] to glam in addition to adding it as a dev-dependency.
Perhaps Uuid should be directly imported vs importing from bevy_utils?

@james7132 james7132 requested a review from MrGVSV January 23, 2024 21:06
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I don't like that we're removing this without marking it deprecated first, but I'm OK with removing it cold-turkey as there should be nothing interacting with it anymore.

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Would have been nice to have first made it #[deprecated], but I don't think the migration should be too bad in most cases— assuming the user isn't already on 0.12.

crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
@MrGVSV MrGVSV added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 23, 2024
@MrGVSV
Copy link
Member

MrGVSV commented Jan 23, 2024

Convert any uses of TypeUuid to use TypePath

@AxiomaticSemantics Could you update the Migration Guide to give users a bit more details on how this is done? Maybe a before/after snippet?

@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 Jan 23, 2024
@AxiomaticSemantics
Copy link
Contributor Author

AxiomaticSemantics commented Jan 23, 2024

Would have been nice to have first made it #[deprecated], but I don't think the migration should be too bad in most cases— assuming the user isn't already on 0.12.

I asked on discord, you thought it was, I was going to ask about this, nothing seemed to reference, but i did also notice it wasn't marked as such. Up to SME/maintainers I guess, seems nothing in the previous guides mention migrating away from it in favour of TypePath.

@AxiomaticSemantics
Copy link
Contributor Author

I don't like that we're removing this without marking it deprecated first, but I'm OK with removing it cold-turkey as there should be nothing interacting with it anymore.

If you'd rather mark it for now, this can hold off until release.

@alice-i-cecile
Copy link
Member

My preference is to remove it outright: there's nothing connected to it.

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.

What's up with the #[rustfmt::skip]? Are they needed? I have a pretty strong preference to remove them unless there's a good reason to have them.

@AxiomaticSemantics
Copy link
Contributor Author

What's up with the #[rustfmt::skip]? Are they needed? I have a pretty strong preference to remove them unless there's a good reason to have them.

I was getting import conflicts with serde and super::serde in the tests, the ::serde prefix was to disambiguate. The annotation on glam I'll remove and test.

@soqb
Copy link
Contributor

soqb commented Jan 24, 2024

What's up with the #[rustfmt::skip]? Are they needed? I have a pretty strong preference to remove them unless there's a good reason to have them.

I was getting import conflicts with serde and super::serde in the tests, the ::serde prefix was to disambiguate. The annotation on glam I'll remove and test.

i think i also had this at one point, can't remember the fix. are you getting the errors through rust-analyzer or through a cli command?

@AxiomaticSemantics
Copy link
Contributor Author

AxiomaticSemantics commented Jan 24, 2024

What's up with the #[rustfmt::skip]? Are they needed? I have a pretty strong preference to remove them unless there's a good reason to have them.

I was getting import conflicts with serde and super::serde in the tests, the ::serde prefix was to disambiguate. The annotation on glam I'll remove and test.

i think i also had this at one point, can't remember the fix. are you getting the errors through rust-analyzer or through a cli command?

I noticed the errors becasuse bevy's CI was failing, no RA in the mix here all cargo invocations.

```error[E0432]: unresolved imports serde::Deserialize, `serde::Serialize`
--> crates/bevy_reflect/src/lib.rs:545:38
|
545 | use serde::{de::DeserializeSeed, Deserialize, Serialize};
| ^^^^^^^^^^^ ^^^^^^^^^
| | |
| | no `Serialize` in `serde`
| | help: a similar name exists in the module: `Serializable`
| no `Deserialize` in `serde`
|
= help: consider importing this trait instead:
::serde::Deserialize
= help: consider importing one of these items instead:
::serde::Serialize
crate::erased_serde::Serialize
erased_serde::Serialize

error[E0432]: unresolved imports glam::quat, glam::vec3, glam::Quat, glam::Vec3
--> crates/bevy_reflect/src/lib.rs:539:16
|
539 | use glam::{quat, vec3, Quat, Vec3};
| ^^^^ ^^^^ ^^^^ ^^^^ no Vec3 in tests::glam
| | | |
| | | no Quat in tests::glam
| | no vec3 in tests::glam
| no quat in tests::glam
|
= help: consider importing one of these items instead:
::glam::Quat
bevy_math::Quat
= help: consider importing one of these items instead:
::glam::Vec3
bevy_math::Vec3
help: a similar name exists in the module
|
539 | use glam::{quat, vec3, quat, Vec3};
| ~~~~
help: a similar name exists in the module
|
539 | use glam::{quat, vec3, Quat, vec3};
| ~~~~

error[E0659]: serde is ambiguous
--> crates/bevy_reflect/src/lib.rs:545:9
|
545 | use serde::{de::DeserializeSeed, Deserialize, Serialize};
| ^^^^^ ambiguous name
|
= note: ambiguous because of a conflict between a name from a glob import and an outer scope during import or macro resolution
= note: serde could refer to a crate passed with --extern
= help: use ::serde to refer to this crate unambiguously
note: serde could also refer to the module imported here
--> crates/bevy_reflect/src/lib.rs:554:9
|
554 | use super::*;
| ^^^^^^^^
= help: consider adding an explicit import of serde to disambiguate
= help: or use self::serde to refer to this module unambiguously
...```

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 25, 2024
Merged via the queue into bevyengine:main with commit 2ebf5a3 Jan 25, 2024
26 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# Objective
TypeUuid is deprecated, remove it.

## Migration Guide
Convert any uses of `#[derive(TypeUuid)]` with `#[derive(TypePath]` for
more complex uses see the relevant
[documentation](https://docs.rs/bevy/latest/bevy/prelude/trait.TypePath.html)
for more information.

---------

Co-authored-by: ebola <dev@axiomatic>
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 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