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.

Sign up for GitHub

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

Jump to bottom

Make drain take a mutable borrow instead of Box<Self> for reflected Map, List, and Set. #15406

Merged
merged 3 commits into from
Sep 30, 2024
Merged

Make drain take a mutable borrow instead of Box<Self> for reflected Map, List, and Set. #15406

merged 3 commits into from
Sep 30, 2024
+42 −34

Conversation

andriyDev
Copy link
Contributor

Objective

Fixes #15185.

Solution

Change drain to take a &mut self for most reflected types.

Some notable exceptions to this change are Array and Tuple. These types don't make sense with drain taking a mutable borrow since they can't get "smaller". Also BTreeMap doesn't have a drain function, so we have to pop elements off one at a time.

Testing

  • The existing tests are sufficient.

Migration Guide

  • reflect::Map, reflect::List, and reflect::Set all now take a &mut self instead of a Box<Self>. Callers of these traits should add &mut before their boxes, and implementers of these traits should update to match.

@andriyDev
Make drain take a mutable borrow instead of Box<Self> for reflect…
5cce145 
…ed `Map`, `List`, and `Set`.

Some notable exceptions to this change are `Array` and `Tuple`. These types don't make sense with `drain` taking a mutable borrow since they can't get "smaller".
@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 C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 24, 2024
MrGVSV
MrGVSV approved these changes Sep 25, 2024
View reviewed changes
crates/bevy_reflect/src/impls/std.rs Show resolved Hide resolved
crates/bevy_reflect/src/list.rs Show resolved Hide resolved
@BenjaminBrienen BenjaminBrienen added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 25, 2024
MrGVSV
MrGVSV reviewed Sep 26, 2024
View reviewed changes
crates/bevy_reflect/src/list.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/map.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/set.rs Outdated Show resolved Hide resolved
pablo-lua
pablo-lua approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

I think this is a major upgrade of the api, its more simple to pass a mut reference. All good, lgtm

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 30, 2024
Merged via the queue into bevyengine:main with commit 04d5685 Sep 30, 2024
26 checks passed
@andriyDev andriyDev deleted the drain branch September 30, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SpecificProtagonist SpecificProtagonist SpecificProtagonist left review comments

@MrGVSV MrGVSV MrGVSV approved these changes

@kristoff3r kristoff3r kristoff3r approved these changes

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

@pablo-lua pablo-lua pablo-lua approved these changes

Assignees
No one assigned
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-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 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make reflect::Map::drain and reflect::List::drain take a mutable borrow instead of Box<Self>.
7 participants
@andriyDev @kristoff3r @SpecificProtagonist @BenjaminBrienen @MrGVSV @pablo-lua @alice-i-cecile