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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - make sub_app return an &App and add sub_app_mut() -> &mut App #3309

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - make sub_app return an &App and add sub_app_mut() -> &mut App #3309

wants to merge 2 commits into from

Conversation

jakobhellermann
Copy link
Contributor

It's sometimes useful to have a reference to an app a sub app at the same time, which is only possible with an immutable reference.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Dec 12, 2021
@mockersf mockersf added A-App Bevy apps and plugins C-Usability A simple quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Dec 12, 2021
@mockersf
Copy link
Member

ci failed to download rust

@mockersf
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Dec 12, 2021
@bors
Try #3309:
8f0eeba
@bors
Copy link
Contributor

bors bot commented Dec 12, 2021

try

Build succeeded:

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 13, 2021
@mockersf
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 15, 2021

Merge conflict.

@cart
Copy link
Member

cart commented Dec 18, 2021

I chose to go against the grain with this api because the mutable variant will be waaaaay more common than the immutable one (as in 99.999% of the time people want a mutable reference). Immutable access is an exception to the rule. I personally think its worth giving the terser / more ergonomic sub_app name to the mutable case.

@aloucks
Copy link
Contributor

aloucks commented Dec 18, 2021

For what it's worth, I thought the old name was confusing. I was looking for something that ended with _mut for gaining mutable access. I consider sub_app(..) -> &mut App to be less ergonomic as it doesn't follow conventions and established patterns.

@Ratysz
Copy link
Contributor

Ratysz commented Dec 19, 2021

Going against convention is a much greater affront to ergonomics than having to type 4 extra symbols (or less, if code completion is in a good mood).

@cart
Copy link
Member

cart commented Dec 20, 2021

I still think something this central deserves a first class name. Immutable access to sub apps is pretty far off the happy path. With a few very exceptional cases, users shouldn't need to think about mutability at all here. Adding _mut increases the cognitive load of the common case to benefit the uncommon case. Conventions aren't hard and fast rules and I'm happy to break them when the cost / benefit of a situation makes sense. My preference is to add a new sub_app_ref api and leave sub_app as is. _ref is a convention used in the std library to indicate a method returns something immutable. That seems suitably clear for doc browsers.

Not putting my foot down here though. Feel free to weigh in / gather support for your preferred approach. Imo this is a pretty clear case of what an exception to the convention should look like (if we are going to allow exceptions). Its worth discussing it as a community and trying to establish consensus so we can quickly resolve these disagreements in the future. If this isn't an exception to the convention, I don't think anything is.

Vote here:
🚀 If you think we should leave this as-is. Exceptions to the x / x_mut convention (in favor of something like x / x_ref) are allowed when the mutable method is the overwhelmingly common use case, in the interest of ergonomics and/or reduction of cognitive load.
🎉 If you think we should strictly follow x, x_mut in cases like this`, in the interest of following conventions and reducing surprises.

(This is an "advisory vote" ... the results won't actually determine what we do, but I will give them weight)

@Ratysz
Copy link
Contributor

Ratysz commented Dec 20, 2021

_ref() only appears alongside _mut(), as much as I can remember right now.

I think we should follow convention as much as we can. I know it's not a rule, and that there are bound to be exceptions that make sense, but this doesn't feel like one.

I don't like the idea of having people learn Rust using libraries and small frameworks (most of which follow convention), then come to Bevy and find that the norms they've gotten used to don't apply.

@aloucks
Copy link
Contributor

aloucks commented Dec 20, 2021

That example isn't really representative of what's happening here though.

Accessors in Rust almost always follow the item(&self) -> &Item and item_mut(&mut self) -> &mut Item pattern

Or use get(&self, id: Id) -> &Item and get_mut(&mut self, id: Id) -> &mut Item for container-like objects.

@aevyrie
Copy link
Member

aevyrie commented Dec 20, 2021

I don't think the esthetics are worth breaking convention in this case. I definitely respect the argument, but as someone who relies on code completion or often times just guessing by following convention, this would trip me up without any perceived gain.

It's also nice to see mutability at a glance when reading someone else's code. I'm not good enough to infer it or remember these exceptions. 🙂

@cart
Copy link
Member

cart commented Dec 23, 2021

Alrighty there is overwhelming support for sub_app_mut, so we will go with that. Thanks to everyone for weighing in!

@cart
Copy link
Member

cart commented Dec 23, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 23, 2021
@jakobhellermann
make sub_app return an &App and add sub_app_mut() -> &mut App (#…
06f832a 
…3309)

It's sometimes useful to have a reference to an app a sub app at the same time, which is only possible with an immutable reference.
@bors
Copy link
Contributor

bors bot commented Dec 23, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Dec 23, 2021
@jakobhellermann
make sub_app return an &App and add sub_app_mut() -> &mut App (#…
5c26839 
…3309)

It's sometimes useful to have a reference to an app a sub app at the same time, which is only possible with an immutable reference.
@bors
Copy link
Contributor

bors bot commented Dec 23, 2021

Build failed:

@cart
Copy link
Member

cart commented Dec 24, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 24, 2021
@jakobhellermann
make sub_app return an &App and add sub_app_mut() -> &mut App (#…
adb3ad3 
…3309)

It's sometimes useful to have a reference to an app a sub app at the same time, which is only possible with an immutable reference.
@bors
Copy link
Contributor

bors bot commented Dec 24, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title make sub_app return an &App and add sub_app_mut() -> &mut App [Merged by Bors] - make sub_app return an &App and add sub_app_mut() -> &mut App Dec 24, 2021
@bors bors bot closed this Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ratysz Ratysz Ratysz approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

Assignees
No one assigned
Labels
A-App Bevy apps and plugins C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jakobhellermann @mockersf @cart @aloucks @Ratysz @aevyrie @alice-i-cecile