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 unnecessary PartialReflect bound on DeserializeWithRegistry #17560

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

aecsocket
Copy link
Contributor

Objective

The new DeserializeWithRegistry trait in 0.15 was designed to be used with reflection, and so has a trait DeserializeWithRegistry: PartialReflect bound. However, this bound is not actually necessary for the trait to function properly. And this PartialReflect bound already exists on:

impl<T: PartialReflect + for<'de> DeserializeWithRegistry<'de>> FromType<T>
    for ReflectDeserializeWithRegistry

So there is no point in constraining the trait itself with this bound as well.

This lets me use DeserializeWithRegistry with non-Reflect types, which I want to do to avoid making a bunch of FooDeserializer structs and impl DeserializeSeed on them.

Solution

Removes this unnecessary bound.

Testing

Trivial change, does not break compilation or bevy_reflect tests.

Migration Guide

DeserializeWithRegistry types are no longer guaranteed to be PartialReflect as well. If you were relying on this type bound, you should add it to your own bounds manually.

- impl<T: DeserializeWithRegistry> Foo for T { .. }
+ impl<T: DeserializeWithRegistry + PartialReflect> Foo for T { .. }

@aecsocket aecsocket force-pushed the feat/unnecessary-bound branch from 0ec026d to 49481d0 Compare January 27, 2025 13:43
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 27, 2025
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 makes sense and I trust the compiler here.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 27, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 27, 2025
@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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 29, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 29, 2025
Merged via the queue into bevyengine:main with commit f232674 Jan 29, 2025
35 checks passed
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-Scenes Serialized ECS data stored on the disk C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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