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

Add Mut::clone_from_if_neq #17019

Merged
merged 1 commit into from
Dec 31, 2024
Merged

Add Mut::clone_from_if_neq #17019

merged 1 commit into from
Dec 31, 2024

Conversation

SOF3
Copy link
Contributor

@SOF3 SOF3 commented Dec 29, 2024 •
edited by alice-i-cecile
Loading

Objective

  • Support more ergonomic conditional updates for types that can be modified by clone_into.

Solution

  • Use ToOwned::clone_into to copy a reference provided by the caller in Mut::clone_from_if_neq.

Testing

  • See doc tests.

BenjaminBrienen
BenjaminBrienen approved these changes Dec 29, 2024
View reviewed changes
crates/bevy_ecs/src/change_detection.rs Outdated
/// # schedule.run(&mut world);
/// # assert!(!message_changed.run((), &mut world));
/// ```
fn clone_if_neq<T>(&mut self, value: &T) -> bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer an enum with 2 variants which are self describing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the return value? This is just to be consistent with set_if_neq. If we were to add an enum for that, I would prefer doing that in a separate PR and change set_if_neq together.

@BenjaminBrienen BenjaminBrienen added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 29, 2024
alice-i-cecile
alice-i-cecile approved these changes Dec 29, 2024
View reviewed changes
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.

I really appreciate the motivation / doc tests on this method: my immediate reaction was "how is this useful"!

@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 Dec 29, 2024
@chescock
Copy link
Contributor

Since this behaves more like clone_from() than clone(), would it make more sense to call it clone_from_if_neq?

@BenjaminBrienen
Copy link
Contributor

CI is still failing

@alice-i-cecile
Copy link
Member

Agreed, I'd prefer clone_from_if_neq. CI failure looks like a missed feature flag.

@SOF3 SOF3 force-pushed the clone-if-neq branch 2 times, most recently from 625a171 to df79451 Compare December 30, 2024 12:46
@alice-i-cecile alice-i-cecile changed the title Add Mut::clone_if_neq Add Mut::clone_from_if_neq Dec 30, 2024
@alice-i-cecile
Copy link
Member

@SOF3 use alloc::borrow::ToOwned; fixed the CI task for me locally. I would push to your branch to add it but I don't have permissions. Ping me when that's done and I'll get this merged.

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Dec 30, 2024
@SOF3
Copy link
Contributor Author

SOF3 commented Dec 31, 2024 •
edited
Loading

image

@alice-i-cecile edits are open to maintainers, not sure why you can't push. I will update it myself later today anyway.

@alice-i-cecile
Copy link
Member

Hmm, very odd. Thanks!

@SOF3
Copy link
Contributor Author

SOF3 commented Dec 31, 2024

um the CI failure doesn't look relevant to this PR

@BenjaminBrienen
Copy link
Contributor

um the CI failure doesn't look relevant to this PR

#17060

@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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 31, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 31, 2024
Merged via the queue into bevyengine:main with commit 3c7fbee Dec 31, 2024
28 of 29 checks passed
@SOF3 SOF3 deleted the clone-if-neq branch December 31, 2024 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

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

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible 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.
4 participants
@SOF3 @chescock @BenjaminBrienen @alice-i-cecile