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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Remove insert_resource_with_id #5608

Closed
wants to merge 7 commits into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Aug 7, 2022

Objective

remove insert_resource_with_id because insert_resource_by_id exists and does almost exactly the same thing

blocked on #5587 because otherwise we will leak a resource when it's inserted

Solution

remove the function and also add a safety invariant of to insert_resource_by_id that the id be valid for the world.

I didn't see any discussion in #4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 馃槄


Changelog

  • safety invariant added to insert_resource_by_id requiring the id to be valid for world

Migration Guide

  • audit any calls to insert_resource_by_id making sure that the id is valid for the world

@BoxyUwU BoxyUwU added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Blocked This cannot move forward until something else changes labels Aug 7, 2022
@jakobhellermann
Copy link
Contributor

jakobhellermann commented Aug 7, 2022

I didn't see any discussion in #4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 馃槄

It just seemed nicer to have less safety
invariants. Especially for user-facing methods I usually try to keep the amount of things you can do wrong to a minimum.

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.com>
@BoxyUwU BoxyUwU removed the S-Blocked This cannot move forward until something else changes label Aug 9, 2022
@cart
Copy link
Member

cart commented Aug 30, 2022

bors r+

鈥 re-add it ... it lives on in our history)
@bors
Copy link
Contributor

bors bot commented Aug 30, 2022

Canceled.

@cart
Copy link
Member

cart commented Aug 30, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 30, 2022
# Objective

remove `insert_resource_with_id` because `insert_resource_by_id` exists and does almost exactly the same thing

blocked on #5587 because otherwise we will leak a resource when it's inserted 

## Solution

remove the function and also add a safety invariant of to `insert_resource_by_id` that the id be valid for the world.

I didn't see any discussion in #4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 馃槄 

---

## Changelog

- safety invariant added to `insert_resource_by_id` requiring the id to be valid for world

## Migration Guide

- audit any calls to `insert_resource_by_id` making sure that the id is valid for the world

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors bors bot changed the title Remove insert_resource_with_id [Merged by Bors] - Remove insert_resource_with_id Aug 30, 2022
@bors bors bot closed this Aug 30, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

remove `insert_resource_with_id` because `insert_resource_by_id` exists and does almost exactly the same thing

blocked on bevyengine#5587 because otherwise we will leak a resource when it's inserted 

## Solution

remove the function and also add a safety invariant of to `insert_resource_by_id` that the id be valid for the world.

I didn't see any discussion in bevyengine#4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 馃槄 

---

## Changelog

- safety invariant added to `insert_resource_by_id` requiring the id to be valid for world

## Migration Guide

- audit any calls to `insert_resource_by_id` making sure that the id is valid for the world

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

remove `insert_resource_with_id` because `insert_resource_by_id` exists and does almost exactly the same thing

blocked on bevyengine#5587 because otherwise we will leak a resource when it's inserted 

## Solution

remove the function and also add a safety invariant of to `insert_resource_by_id` that the id be valid for the world.

I didn't see any discussion in bevyengine#4447 about this safety invariant being left off in favor of a panic so I'm curious if there was one or if it just seemed nicer to have less safety invariants for callers to uphold 馃槄 

---

## Changelog

- safety invariant added to `insert_resource_by_id` requiring the id to be valid for world

## Migration Guide

- audit any calls to `insert_resource_by_id` making sure that the id is valid for the world

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants