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.

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

The iter_many_mut documentation is confused #5506

Closed
nicopap opened this issue Jul 31, 2022 · 2 comments
Closed

The iter_many_mut documentation is confused #5506

nicopap opened this issue Jul 31, 2022 · 2 comments
Assignees
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation

Comments

@nicopap
Copy link
Contributor

nicopap commented Jul 31, 2022

Calls a closure on each result of Query where the entities match.

The documentation talks about calling a "closure", but iter_many_mut doesn't have a closure argument, as the example shows. https://docs.rs/bevy/latest/bevy/ecs/system/struct.Query.html#method.iter_many_mut

@nicopap nicopap added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Jul 31, 2022
@Nilirad
Copy link
Contributor

Nilirad commented Jul 31, 2022

I'll rebase #4989 to main and add more appropriate doc comments.

@ian-h-chamberlain
Copy link
Contributor

To add to this, it would be helpful to add a mention that the returned value is not actually an Iterator. I was confused by the errors when I tried

for foo in q.iter_many_mut(&entities) {}

The comment on the Iterator impl for QueryManyIter explains this a bit:

/// Iterator type is intentionally implemented only for read-only access.
/// Doing so for mutable references would be unsound,
/// because calling `next` multiple times would allow multiple owned references to the same data to exist.

but it might be good to mention as well on iter_many_mut as well, to explain the need for this usage (which can easily be misread as the more common while let Some(_) = it.next():

let mut it = q.iter_many_mut(&entities)
while let Some(foo) = it.fetch_next() {}

bors bot pushed a commit that referenced this issue Sep 2, 2022
# Objective

- Increase consistency across documentation of `Query` methods.
- Fixes #5506

## Solution

- See #4989. This PR is derived from it. It just includes changes to the `Query` methods' docs.
@bors bors bot closed this as completed in dfeb63e Sep 2, 2022
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
# Objective

- Increase consistency across documentation of `Query` methods.
- Fixes bevyengine#5506

## Solution

- See bevyengine#4989. This PR is derived from it. It just includes changes to the `Query` methods' docs.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

- Increase consistency across documentation of `Query` methods.
- Fixes bevyengine#5506

## Solution

- See bevyengine#4989. This PR is derived from it. It just includes changes to the `Query` methods' docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation
Projects
None yet
3 participants