[Merged by Bors] - Make get_resource (and friends) infallible #4047

alice-i-cecile · 2022-02-26T19:25:02Z

Objective

In the large majority of cases, users were calling .unwrap() immediately after .get_resource.
Attempting to add more helpful error messages here resulted in endless manual boilerplate (see Tracking issue: Replace unwrap with expect #3899 and the linked PRs).

Solution

Add an infallible variant named .resource and so on.
Use these infallible variants over .get_resource().unwrap() across the code base.

Notes

I did not provide equivalent methods on WorldCell, in favor of removing it entirely in #3939.

Migration Guide

Infallible variants of .get_resource have been added that implicitly panic, rather than needing to be unwrapped.

Replace world.get_resource::<Foo>().unwrap() with world.resource::<Foo>().

Impact

.unwrap search results before: 1084
.unwrap search results after: 942
internal unwrap_or_else calls added: 4
trivial unwrap calls removed from tests and code: 146
uses of the new try_get_resource API: 11
percentage of the time the unwrapping API was used internally: 93%

alice-i-cecile · 2022-02-26T19:28:32Z

Note to reviewers: all of the non-trivial changes are found in World::mod.res. The rest is just fallout from this change.

DJMcNab · 2022-02-26T19:42:29Z

Haven't reviewed the code, but this change seems sensible.

However, the get_resource name would conventionally denote a fallible operation (and so try_get_resource suffers from meaninglessness). I'm not sure what the solution looks like - resource and get_resource would probably be most idiomatic, but the name resource isn't very expressive.

ghost

Just some small nits. Very lovely change!

crates/bevy_ecs/src/world/mod.rs

Co-authored-by: KDecay <KDecayMusic@protonmail.com>

ghost · 2022-02-26T20:16:54Z

However, the get_resource name would conventionally denote a fallible operation (and so try_get_resource suffers from meaninglessness). I'm not sure what the solution looks like - resource and get_resource would probably be most idiomatic, but the name resource isn't very expressive.

Not sure what the most idiomatic way to do this would be, but IMO get_resource should panic and try_get_resource should return an Option or Result. This is also the way that it is currently implemented in this PR. Usually get is left out for accessing members, but as you already said that wouldn't be very expressive and shouldn't be done in this case.

get_resource implies that you know that it exists and you will get a reference to it.
try_get_resource implies that you don't know if it exists and you may or may not get a reference to it.

alice-i-cecile · 2022-02-26T20:33:10Z

However, the get_resource name would conventionally denote a fallible operation (and so try_get_resource suffers from meaninglessness). I'm not sure what the solution looks like - resource and get_resource would probably be most idiomatic, but the name resource isn't very expressive.

Yeah, that would be consistent with the World::entity API. I could go either way on it, but wanted some bikeshedding on whether this would be a good naming strategy.

jakobhellermann · 2022-02-26T21:28:21Z

To add to the bikeshedding, I feel like .resource() -> T and .get_resource() -> Option<T> is the best naming, it is consistent with our use of .sub_app() and .get_sub_app(), get_ is commonly used for methods returning an option and let time = world.resource::<Time>(); is quite readable in my opinion.

mcobzarenco · 2022-02-26T22:30:18Z

To add to the bikeshedding, I feel like .resource() -> T and .get_resource() -> Option is the best naming,

To further add to the bikeshedding, I agree and feel this is the common rust convention, e.g. used in std. I would expect get_foo() to return an Option and foo() to potentially panic. This is not consistently done in bevy today, e.g. whereas World::entity API uses this convention, Mesh::attribute returns an Option (which tripped me up initially). Whichever convention, it'd be even nicer to use it consistently.

alice-i-cecile · 2022-02-26T22:43:16Z

To add to the bikeshedding, I feel like .resource() -> T and .get_resource() -> Option is the best naming

Great, I'll make this change. This also ensures that this change is non-breaking, which is very nice.

mcobzarenco

💯 for this quality of life improvement PR

crates/bevy_ecs/src/world/mod.rs

Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>

cart · 2022-02-27T22:37:02Z

bors r+

# Objective - In the large majority of cases, users were calling `.unwrap()` immediately after `.get_resource`. - Attempting to add more helpful error messages here resulted in endless manual boilerplate (see #3899 and the linked PRs). ## Solution - Add an infallible variant named `.resource` and so on. - Use these infallible variants over `.get_resource().unwrap()` across the code base. ## Notes I did not provide equivalent methods on `WorldCell`, in favor of removing it entirely in #3939. ## Migration Guide Infallible variants of `.get_resource` have been added that implicitly panic, rather than needing to be unwrapped. Replace `world.get_resource::<Foo>().unwrap()` with `world.resource::<Foo>()`. ## Impact - `.unwrap` search results before: 1084 - `.unwrap` search results after: 942 - internal `unwrap_or_else` calls added: 4 - trivial unwrap calls removed from tests and code: 146 - uses of the new `try_get_resource` API: 11 - percentage of the time the unwrapping API was used internally: 93%

bors · 2022-02-27T22:52:22Z

Pull request successfully merged into main.

Build succeeded:

- In the large majority of cases, users were calling `.unwrap()` immediately after `.get_resource`. - Attempting to add more helpful error messages here resulted in endless manual boilerplate (see bevyengine#3899 and the linked PRs). - Add an infallible variant named `.resource` and so on. - Use these infallible variants over `.get_resource().unwrap()` across the code base. I did not provide equivalent methods on `WorldCell`, in favor of removing it entirely in bevyengine#3939. Infallible variants of `.get_resource` have been added that implicitly panic, rather than needing to be unwrapped. Replace `world.get_resource::<Foo>().unwrap()` with `world.resource::<Foo>()`. - `.unwrap` search results before: 1084 - `.unwrap` search results after: 942 - internal `unwrap_or_else` calls added: 4 - trivial unwrap calls removed from tests and code: 146 - uses of the new `try_get_resource` API: 11 - percentage of the time the unwrapping API was used internally: 93%

alice-i-cecile added 7 commits February 26, 2022 13:16

Updated docs

29c9ea3

Try get resource methods

96463a6

Update usages of .get_resource

c8294af

Remove pointless WorldCell

6a603f7

Finish updating usages of .get_resources

7b4eb3d

More helpful error messages

0bd5bdb

Change get_non_send_resource behavior to match

69b82c2

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 26, 2022

alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Feb 26, 2022

Remove unwrap in test

08aa672

ghost mentioned this pull request Feb 26, 2022

Tracking issue: Replace unwrap with expect #3899

Closed

31 tasks

ghost suggested changes Feb 26, 2022

View reviewed changes

Nits from code review

2550ec3

Co-authored-by: KDecay <KDecayMusic@protonmail.com>

Remove more unwraps from examples

b7f0af7

Unwraps in macro...

a39d895

Rename methods to match convention

74f50a3

mcobzarenco reviewed Feb 27, 2022

View reviewed changes

crates/bevy_ecs/src/world/mod.rs Show resolved Hide resolved

alice-i-cecile added 3 commits February 26, 2022 19:29

Move stray safety comment

5444ff0

Fix assorted compilation errors and tests

5aa4b48

Remove expect in example

3f3b495

TheRawMeatball approved these changes Feb 27, 2022

View reviewed changes

Weasy666 reviewed Feb 27, 2022

View reviewed changes

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved

Improve error messages

d63c760

MinerSebas suggested changes Feb 27, 2022

View reviewed changes

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved

Use #[track_caller]

d36944d

Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>

bors bot changed the title ~~Make get_resource (and friends) infallible~~ [Merged by Bors] - Make get_resource (and friends) infallible Feb 27, 2022

bors bot closed this Feb 27, 2022

ghost mentioned this pull request Feb 28, 2022

Replace unwraps with expects in bevy_pbr #4046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Make get_resource (and friends) infallible #4047

[Merged by Bors] - Make get_resource (and friends) infallible #4047

alice-i-cecile commented Feb 26, 2022 •

edited

Loading

alice-i-cecile commented Feb 26, 2022 •

edited

Loading

DJMcNab commented Feb 26, 2022

ghost left a comment

ghost commented Feb 26, 2022 •

edited by ghost

Loading

alice-i-cecile commented Feb 26, 2022

jakobhellermann commented Feb 26, 2022

mcobzarenco commented Feb 26, 2022 •

edited

Loading

alice-i-cecile commented Feb 26, 2022

mcobzarenco left a comment

cart commented Feb 27, 2022

bors bot commented Feb 27, 2022

[Merged by Bors] - Make get_resource (and friends) infallible #4047

[Merged by Bors] - Make get_resource (and friends) infallible #4047

Conversation

alice-i-cecile commented Feb 26, 2022 • edited Loading

Objective

Solution

Notes

Migration Guide

Impact

alice-i-cecile commented Feb 26, 2022 • edited Loading

DJMcNab commented Feb 26, 2022

ghost left a comment

Choose a reason for hiding this comment

ghost commented Feb 26, 2022 • edited by ghost Loading

alice-i-cecile commented Feb 26, 2022

jakobhellermann commented Feb 26, 2022

mcobzarenco commented Feb 26, 2022 • edited Loading

alice-i-cecile commented Feb 26, 2022

mcobzarenco left a comment

Choose a reason for hiding this comment

cart commented Feb 27, 2022

bors bot commented Feb 27, 2022

alice-i-cecile commented Feb 26, 2022 •

edited

Loading

alice-i-cecile commented Feb 26, 2022 •

edited

Loading

ghost commented Feb 26, 2022 •

edited by ghost

Loading

mcobzarenco commented Feb 26, 2022 •

edited

Loading