bevy
bevy copied to clipboard
bevyengine
ChildBuilder methods should take IntoIterator, rather than a slice type
It's generally better to use an iterator type instead of forcing someone to use a `slice`. Since it's not always guaranteed that the children entities will be in an array-like container.
fn replace_children(&mut self, children: impl IntoIterator<Item = Entity>) -> &mut Self {
Originally posted by @NathanSWard in https://github.com/bevyengine/bevy/pull/6035#discussion_r981546329
Given the fact that there's already a pull request concerning updating this, is there anything i could help with here? if not, that's fine, just stumbled upon this project and thought it looked interesting.
#7244 is in progress: at this point I would suggest either adopting the work there (rebase the commits and make your own PR sharing credit) or making PRs to the author's branch and pinging them on the thread :)
I started from https://github.com/bevyengine/bevy/pull/7244 and created https://github.com/bevyengine/bevy/pull/7939. But now I'm starting to wonder whether using an iterator that is consumed improves upon the ergonomics of passing an &[Entity] slice around. Two passes on the PR were made, one with impl IntoIterator<Item = Entity> and another with impl IntoIterator<Item = impl Borrow<Entity>>, but neither approach feels quite right.
First, downstream code will want to use the children argument more than once, and since the iterator will be consumed, this leads to collecting it into a vec deeper in the call stack. Using impl IntoIterator<Item = Entity> further consumes the entries in the iterator, which might make sense in some cases, but perhaps not all. An alternative that has been suggested, impl IntoIterator<Item = impl Borrow<Entity>>, has other issues in terms of ergonomics, and still requires collecting an intermediate vec.
My knowledge of Rust is shaky enough that I don't have well formed opinions on these questions. But the current state of the PR does not seem quite right, either. So I'm happy to leave this story for anyone else who wants to take it.
That's a reasonable argument, but this is pretty hot code. I'm very reluctant to require additional allocations.
That makes sense. If you can think of a way to avoid the intermediate vecs while still passing in an iterator to the trait methods, then perhaps the PR can be salvaged. Otherwise I'm inclined to close the PR.