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

[jakobhellermann]

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.

[mockersf]

ci failed to download rust

[mockersf]

bors try

[bors]

try

Build succeeded:

• build-wasm (nightly, ubuntu-latest)
• check-missing-examples-in-docs
• build-wasm (stable, ubuntu-latest)
• build (stable, macos-latest)
• build-android
• check-doc
• ci
• build (stable, windows-latest)
• markdownlint
• run-examples
• check-unused-dependencies
• build (nightly, ubuntu-latest)
• check-markdown-links
• build (stable, ubuntu-latest)
• check-benches

[mockersf]

bors r+

[bors]

Merge conflict.

[cart]

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]

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]

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]

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]

_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]

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]

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]

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

[cart]

bors r+

[bors]

Build failed (retrying...):

• build-wasm (nightly, ubuntu-latest)

[bors]

Build failed:

• build-wasm (nightly, ubuntu-latest)

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (stable, ubuntu-latest)
• check-benches
• check-doc
• check-markdown-links
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples