-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Rename clear_child* to detach_child* #21470
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
Rename clear_child* to detach_child* #21470
Conversation
does not rename ui_surface.try_remove_children. rename remove_children to detach_all_children to avoid naming conflicts.
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
Decided to use the deprecation mechanism, because it was mentioned in the migration guide guide, and a lot of downstream projects will likely be affected. |
alice-i-cecile
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thank you!
janis-bhm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Should this same change be applied to *_related methods that these methods specialise, too? I think the same rationale applies to them as well, but maybe that is for a follow up PR, so it shouldn't block this one.
|
Also, the methods |
# Objective Fixes bevyengine#21329 - The naming of the `clear_children` function could be misunderstood, so we suggest to rename it. The current `clear_children` function could be understood to despawn the children, which it does not do. To emphasize the difference between `clear` and `despawn`, we can rename `clear` to `detach`. So we arrive at the final naming of `despawn` and `detach`. ## Solution To improve this, we could rename `clear_children` to `detach_children` and all other `clear`/`remove` functions too. I also suggest to rename all `remove_child*` functions for the same reason. This renaming resulted in a confict because two methods now had the same name: - `detach_children(self)`, previously `clear_children` - `detach_children(self, children: &[Entity])`, previously `remove_children` To solve this, I propose we rename the former to `detach_all_children`. This was chosen because it has the least impact, considering that all methods do take a slice of children, except this one. Changing the `insert_*` and `add_*` functions should be discussed. I think there is less potential for confusion, because they require you to pass an existing entity. Therefore it should be clear that this does not spawn anything. Note: The function `ui_surface.try_remove_children` has not been renamed. ## Testing Currently running the tests, but there are unrelated compile errors in wgpu-hal blocking me (I am on windows). For this reason, this is a draft pull request. Feel free to provide feedback already, while I am trying to make the tests run. ## Discussion As proposed in this PR, this would be a breaking change and will likely affect a large number of downstream projects directly. If you think this should be handled differently, via a `#[deprecated]` flag, I can rework the code to do that. Is there no documentation to update? The repo yielded no more search results than this. --------- Co-authored-by: Alice Cecile <[email protected]>
Objective
Fixes #21329 - The naming of the
clear_childrenfunction could be misunderstood, so we suggest to rename it.The current
clear_childrenfunction could be understood to despawn the children, which it does not do. To emphasize the difference betweenclearanddespawn, we can renamecleartodetach. So we arrive at the final naming ofdespawnanddetach.Solution
To improve this, we could rename
clear_childrentodetach_childrenand all otherclear/removefunctions too. I also suggest to rename allremove_child*functions for the same reason.This renaming resulted in a confict because two methods now had the same name:
detach_children(self), previouslyclear_childrendetach_children(self, children: &[Entity]), previouslyremove_childrenTo solve this, I propose we rename the former to
detach_all_children. This was chosen because it has the least impact, considering that all methods do take a slice of children, except this one.Changing the
insert_*andadd_*functions should be discussed. I think there is less potential for confusion, because they require you to pass an existing entity. Therefore it should be clear that this does not spawn anything.Note: The function
ui_surface.try_remove_childrenhas not been renamed.Testing
Currently running the tests, but there are unrelated compile errors in wgpu-hal blocking me (I am on windows).
For this reason, this is a draft pull request. Feel free to provide feedback already, while I am trying to make the tests run.
Discussion
As proposed in this PR, this would be a breaking change and will likely affect a large number of downstream projects directly. If you think this should be handled differently, via a
#[deprecated]flag, I can rework the code to do that.Is there no documentation to update? The repo yielded no more search results than this.