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

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

Closed

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Feb 26, 2022

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%

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 26, 2022
@alice-i-cecile 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
@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Feb 26, 2022

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

@ghost ghost mentioned this pull request Feb 26, 2022
31 tasks
@DJMcNab
Copy link
Member

DJMcNab commented Feb 26, 2022

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.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small nits. Very lovely change!

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
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: KDecay <KDecayMusic@protonmail.com>
@ghost
Copy link

ghost commented Feb 26, 2022

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

mcobzarenco commented Feb 26, 2022

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
Copy link
Member Author

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.

Copy link
Contributor

@mcobzarenco mcobzarenco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 for this quality of life improvement PR

crates/bevy_ecs/src/world/mod.rs 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
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
@cart
Copy link
Member

cart commented Feb 27, 2022

bors r+

bors bot pushed a commit that referenced this pull request Feb 27, 2022
# 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 bors bot changed the title Make get_resource (and friends) infallible [Merged by Bors] - Make get_resource (and friends) infallible Feb 27, 2022
@bors bors bot closed this Feb 27, 2022
kurtkuehnert pushed a commit to kurtkuehnert/bevy that referenced this pull request Mar 6, 2022
- 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%
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants