[Merged by Bors] - Remove ExactSizeIterator from QueryCombinationIter#5895
Closed
ottah wants to merge 2 commits intobevyengine:mainfrom
ottah:5846-QueryCominationIter-remove-ExactSizeIterator
Closed
[Merged by Bors] - Remove ExactSizeIterator from QueryCombinationIter#5895ottah wants to merge 2 commits intobevyengine:mainfrom ottah:5846-QueryCominationIter-remove-ExactSizeIterator
ottah wants to merge 2 commits intobevyengine:mainfrom
ottah:5846-QueryCominationIter-remove-ExactSizeIterator
Conversation
nicopap
reviewed
Sep 6, 2022
Comment on lines
-116
to
-121
| for query: {query_type} | ||
| expected: {expected_size} | ||
| len(): {len} | ||
| size_hint().0: {size_hint_0} | ||
| size_hint().1: {size_hint_1:?} | ||
| count(): {count}"# |
Contributor
There was a problem hiding this comment.
The test doesn't only check for len being consistent, but also size_hint being equivalent to count. Removing the ExactSize impl doesn't imply the size_hint cannot be accurate.
Note that this test both checks both the QueryIter and QueryCombinationIter counts. I suggest extracting the assert_combination, assert_all_sizes_equal, assert_all_sizes_iterator_equal functions, split this test in two, and not test len for QueryCombinationIter.
Also if you are running tests, I recommend you use cargo test --package bevy_ecs query_filtered_exactsizeiterator_len (or whatever new name you come up with) this will save you time.
Contributor
Author
There was a problem hiding this comment.
I think I've implemented what you suggested by splitting to 2 tests. Is query_filtered_exactsizeiterator_len correct?
djeedai
added
C-Bug
Contributor
|
I'd suggest you edit the PR comment to improve the migration guide. Ideally it contains actionable instructions. Here is an example:
|
ottah
changed the title
nicopap
approved these changes
Sep 10, 2022
crates/bevy_ecs/src/query/mod.rs
Outdated
Comment on lines
+73
to
+79
| r#"query declared sizes: | ||
| for query: {query_type} | ||
| expected: {expected_size} | ||
| len: {len} | ||
| size_hint().0: {size_hint_0} | ||
| size_hint().1: {size_hint_1:?} | ||
| count(): {count}"# |
Contributor
There was a problem hiding this comment.
Suggested change
| r#"query declared sizes: | |
| for query: {query_type} | |
| expected: {expected_size} | |
| len: {len} | |
| size_hint().0: {size_hint_0} | |
| size_hint().1: {size_hint_1:?} | |
| count(): {count}"# | |
| "query declared sizes: \ | |
| for query: {query_type} \ | |
| expected: {expected_size} \ | |
| len: {len} \ | |
| size_hint().0: {size_hint_0} \ | |
| size_hint().1: {size_hint_1:?} \ | |
| count(): {count}" |
Nit, non blocking, but formatting can be improved
crates/bevy_ecs/src/query/mod.rs
Outdated
Comment on lines
210
to
215
| r#"query declared sizes: | ||
| for query: {query_type} | ||
| for query: {query_type} | ||
| expected: {expected_size} | ||
| len(): {len} | ||
| size_hint().0: {size_hint_0} | ||
| size_hint().1: {size_hint_1:?} | ||
| count(): {count}"# |
Contributor
There was a problem hiding this comment.
Nit: same as previous
"query declared sizes: \
for query: {query_type} \
expected: {expected_size} \
size_hint().0: {size_hint_0} \
size_hint().1: {size_hint_1:?} \
count(): {count}"
Contributor
Author
|
@nicopap Hey I updated the PR due to fix conflicts from |
james7132
approved these changes
Oct 20, 2022
james7132
added
the
S-Ready-For-Final-Review
Member
|
Oh no, more merge conflicts. I'll merge it ASAP once those are resolved. |
QueryCombinationIter can have sizes greater than usize::MAX. - Remove implementation of ExactSizeIterator - Remove UI compile fail tests for ExactSizeIterator - Align println! strings Fixes #5846
WorldQuery -> ReadOnlyWorldQuery world.spawn().insert_bundle(...) -> world.spawn(...)
Member
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Oct 24, 2022
# Objective - `QueryCombinationIter` can have sizes greater than `usize::MAX`. - Fixes #5846 ## Solution - Only the implementation of `ExactSizeIterator` has been removed. Instead of using `query_combination.len()`, you can use `query_combination.size_hint().0` to get the same value as before. --- ## Migration Guide - Switch to using other methods of getting the length.
Contributor
|
Pull request successfully merged into main. Build succeeded:
|
bors
bot
changed the title
james7132
pushed a commit
to james7132/bevy
that referenced
this pull request
Oct 28, 2022
# Objective - `QueryCombinationIter` can have sizes greater than `usize::MAX`. - Fixes bevyengine#5846 ## Solution - Only the implementation of `ExactSizeIterator` has been removed. Instead of using `query_combination.len()`, you can use `query_combination.size_hint().0` to get the same value as before. --- ## Migration Guide - Switch to using other methods of getting the length.
Pietrek14
pushed a commit
to Pietrek14/bevy
that referenced
this pull request
Dec 17, 2022
# Objective - `QueryCombinationIter` can have sizes greater than `usize::MAX`. - Fixes bevyengine#5846 ## Solution - Only the implementation of `ExactSizeIterator` has been removed. Instead of using `query_combination.len()`, you can use `query_combination.size_hint().0` to get the same value as before. --- ## Migration Guide - Switch to using other methods of getting the length.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective - `QueryCombinationIter` can have sizes greater than `usize::MAX`. - Fixes bevyengine#5846 ## Solution - Only the implementation of `ExactSizeIterator` has been removed. Instead of using `query_combination.len()`, you can use `query_combination.size_hint().0` to get the same value as before. --- ## Migration Guide - Switch to using other methods of getting the length.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
QueryCombinationItercan have sizes greater thanusize::MAX.Solution
ExactSizeIteratorhas been removed. Instead of usingquery_combination.len(), you can usequery_combination.size_hint().0to get the same value as before.Migration Guide