Skip to content

bevy_reflect: Introduce reflect_clone_and_take. #19944

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

bevy_reflect: Introduce reflect_clone_and_take. #19944

wants to merge 2 commits into from

Conversation

nnethercote
Copy link
Contributor

Objective

There is a pattern that appears in multiple places, involving reflect_clone, followed by take, followed by map_err that produces a FailedDowncast in a particular form.

Solution

Introduces reflect_clone_and_take, which factors out the repeated code.

Testing

cargo run -p ci

@nnethercote
Copy link
Contributor Author

reflect_clone_and_take must be pub because it appears in generated code produced by derive(Reflect). I wasn't entirely happy with this but I couldn't see a way around it. I made it a free function but it could also be a provided method on PartialReflect.

@nnethercote nnethercote changed the title Introduce reflect_clone_and_take. bevy_reflect: Introduce reflect_clone_and_take. Jul 4, 2025
@alice-i-cecile
Copy link
Member

I would prefer a method. It's less noisy in the docs.

@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times A-Reflection Runtime information about types S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 4, 2025
@nnethercote nnethercote force-pushed the reflect_clone_and_take branch from 87f364c to 3c4207c Compare July 4, 2025 10:12
@nnethercote
Copy link
Contributor Author

I would prefer a method

done

alice-i-cecile
alice-i-cecile reviewed Jul 4, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Jul 4, 2025
View reviewed changes
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Code-Quality A section of code that is hard to understand or change X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 4, 2025
SkiFire13
SkiFire13 approved these changes Jul 4, 2025
View reviewed changes
MrGVSV
MrGVSV approved these changes Jul 4, 2025
View reviewed changes
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.

Just a couple doc nits, but overall looks good!

@tychedelia tychedelia 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 Jul 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@SkiFire13 SkiFire13 SkiFire13 approved these changes

@MrGVSV MrGVSV MrGVSV approved these changes

Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times 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 X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nnethercote @alice-i-cecile @SkiFire13 @MrGVSV @tychedelia