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

reflect: insert and remove methods for List #7061

Closed
soqb opened this issue Dec 29, 2022 · 3 comments
Closed

reflect: insert and remove methods for List #7061

soqb opened this issue Dec 29, 2022 · 3 comments
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@soqb
Copy link
Contributor

soqb commented Dec 29, 2022

What problem does this solve or what need does it fill?

Currently, the reflect List trait implements only push and pop functionality. There is no way to insert or remove elements other than from/to the end other than by downcasting. This is especially notable using types like VecDeque with reflection.

What solution would you like?

Add insert and remove methods for List and implement them.
Give push and pop default implementations using on the insert/remove and len methods.

What alternative(s) have you considered?

We might not need to implement the methods by default but since we can I believe we should as to not put to much burden on the implementor.

Additional context

Discussed first on discord.

@soqb soqb added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Dec 29, 2022
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Dec 29, 2022
@alice-i-cecile
Copy link
Member

Yep, if we can have a default impl for this I'm on board. Probably warn in the trait docs that manual impls may be faster.

@soqb
Copy link
Contributor Author

soqb commented Dec 29, 2022

trying to implement this. for the signature of insert and remove, should it match Vec where insertion and removal panic when the index is out of bounds or have them as fallible operations: fn insert(...) -> bool and fn remove(...) -> Option<Box<dyn Reflect>>?

edit: push and pop both match Vec but push never panics or fails and pop returns an Option.

@MrGVSV
Copy link
Member

MrGVSV commented Dec 29, 2022

trying to implement this. for the signature of insert and remove, should it match Vec where insertion and removal panic when the index is out of bounds or have them as fallible operations: fn insert(...) -> bool and fn remove(...) -> Option<Box>?

I’m in favor of just matching Vec. We have ways for users to reflectively perform their own bounds checking to ensure they don't panic, so I think it's fine to match that behavior

@bors bors bot closed this as completed in 1b9c156 Jan 9, 2023
james7132 pushed a commit to james7132/bevy that referenced this issue Jan 21, 2023
# Objective

- Fixes bevyengine#7061

## Solution

- Add and implement `insert` and `remove` methods for `List`.

---

## Changelog

- Added `insert` and `remove` methods to `List`.
- Changed the `push` and `pop` methods on `List` to have default implementations.

## Migration Guide

- Manual implementors of `List` need to implement the new methods `insert` and `remove` and 
consider whether to use the new default implementation of `push` and `pop`.

Co-authored-by: radiish <[email protected]>
alradish pushed a commit to alradish/bevy that referenced this issue Jan 22, 2023
# Objective

- Fixes bevyengine#7061

## Solution

- Add and implement `insert` and `remove` methods for `List`.

---

## Changelog

- Added `insert` and `remove` methods to `List`.
- Changed the `push` and `pop` methods on `List` to have default implementations.

## Migration Guide

- Manual implementors of `List` need to implement the new methods `insert` and `remove` and 
consider whether to use the new default implementation of `push` and `pop`.

Co-authored-by: radiish <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

- Fixes bevyengine#7061

## Solution

- Add and implement `insert` and `remove` methods for `List`.

---

## Changelog

- Added `insert` and `remove` methods to `List`.
- Changed the `push` and `pop` methods on `List` to have default implementations.

## Migration Guide

- Manual implementors of `List` need to implement the new methods `insert` and `remove` and 
consider whether to use the new default implementation of `push` and `pop`.

Co-authored-by: radiish <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants