Add get_at_mut to bevy_reflect::Map trait #8691
Merged
Conversation
This file contains 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
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
djeedai
added
C-Feature
djeedai
approved these changes
May 27, 2023
MrGVSV
reviewed
May 27, 2023
crates/bevy_reflect/src/map.rs
Outdated
| @@ -49,9 +49,12 @@ pub trait Map: Reflect { | |||
| /// If no value is associated with `key`, returns `None`. | |||
| fn get_mut(&mut self, key: &dyn Reflect) -> Option<&mut dyn Reflect>; | |||
|
|
|||
| /// Returns the key-value pair at `index` by reference, or `None` if out of bounds. | |||
| /// Returns the key-value pair at `index` by references, or `None` if out of bounds. | |||
There was a problem hiding this comment.
Nit: I think the original was actually correct (could be wrong)
Suggested change
| /// Returns the key-value pair at `index` by references, or `None` if out of bounds. | |
| /// Returns the key-value pair at `index` by reference, or `None` if out of bounds. |
crates/bevy_reflect/src/map.rs
Outdated
| fn get_at(&self, index: usize) -> Option<(&dyn Reflect, &dyn Reflect)>; | ||
|
|
||
| /// Returns the key-value pair at `index` by references where value reference is mutable, or `None` if out of bounds. |
There was a problem hiding this comment.
Nit: Possible re-phrasing:
Suggested change
| /// Returns the key-value pair at `index` by references where value reference is mutable, or `None` if out of bounds. | |
| /// Returns the key-value pair at `index` by reference where the value is a mutable reference, or `None` if out of bounds. |
There was a problem hiding this comment.
Could we also add a test that ensures get_at_mut is working as intended?
Edit: I guess we don't actually have one for get_at 😅. Would you be able to add one for that as well?
There was a problem hiding this comment.
Sure! I tried to write tests as helpful as possible, but still not sure whether they are. Looking forward to your feedback on that
MrGVSV
approved these changes
May 29, 2023
MrGVSV
added
the
S-Ready-For-Final-Review
alice-i-cecile
changed the title
Selene-Amanita
added
the
M-Needs-Migration-Guide
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Fixes #8596
Solution
Change interface of the trait Map. Adjust implementations of this trait
Changelog
Changed
Added
Map::get_at_mutMigration Guide
Every implementor of Map trait would need to implement
get_at_mut. Which, judging by changes in this PR, should be fairly trivial.