Improve clarity of condition.rs doc examples

[IRSMsoso]

Objective

A variable named app calling add_system is potentially confusing since the variable is actually of type Schedule, and thus doesn't specify a schedule in the call.

Fixes #11814
Alternative to #11815

Solution

Rename app to schedule to match the actual type being used.

Alternatives

• Introduce a mock ScheduleLabel for each non compile_fail doc example, and mock app.add_systems with it.
• Do nothing. It's not all that confusing and I think only people looking closely at it will be confused by the lack of a schedule parameter.

[hymm]

Maybe use a Schedules instead of Schedule. That at least has the same function signature for add_systems.

[IRSMsoso]

Maybe use a Schedules instead of Schedule. That at least has the same function signature for add_systems.

I tried schedules.add_systems(Update, ...), but ScheduleLabels like Update are part of bevy_app. Maybe declare a new Update ScheduleLabel in each example?

Also, the specific Schedule must be retrieved from schedules.get_mut(ScheduleLabel) before they can be run with schedule.run(&mut world). For examples where the schedule.run(&mut world) is visible, like this example:

    /// # use bevy_ecs::prelude::*;
    /// # #[derive(Resource, Default, PartialEq)]
    /// # struct Counter(u8);
    /// # let mut schedules = Schedules::default();
    /// # let mut world = World::new();
    /// schedules.add_systems(
    ///     Update,
    ///     // `resource_exists_and_equals` will only return true
    ///     // if the given resource exists and equals the given value
    ///     my_system.run_if(resource_exists_and_equals(Counter(0))),
    /// );
    ///
    /// fn my_system(mut counter: ResMut<Counter>) {
    ///     counter.0 += 1;
    /// }
    ///
    /// let schedule = schedules.get_mut(Update).unwrap();
    ///
    /// // `Counter` hasn't been added so `my_system` can't run
    /// schedule.run(&mut world);
    /// world.init_resource::<Counter>();
    ///
    /// // `Counter` is `0` so `my_system` can run
    /// schedule.run(&mut world);
    /// assert_eq!(world.resource::<Counter>().0, 1);
    ///
    /// // `Counter` is no longer `0` so `my_system` won't run
    /// schedule.run(&mut world);
    /// assert_eq!(world.resource::<Counter>().0, 1);

I opted to keep the let schedule = schedules.get_mut(Update).unwrap() unhidden to show that it's a different type being used, but maybe this is confusing? Alternatively, we could just name both the Schedule and Schedules variables schedule or schedules or even app and rebind the variable to the other type and don't show it.

/// # let app = app.get_mut(Update).unwrap();

It seems dishonest to me, but maybe it makes the example clearer to view. Not sure if that's acceptable.🤷‍♂️

[IRSMsoso]

So I believe the questions I have are:

• Should I add a dummy Update ScheduleLabel to each example to facilitate using Schedules::add_systems for the same function signature as App::add_systems?
  • If so, should I obfuscate the naming and pretend that it's an app? It would look the same as App from an outside perspective, but the actual implementation would use ScheduleLabels with a dummy Update label.

[alice-i-cecile]

Should I add a dummy Update ScheduleLabel to each example to facilitate using Schedules::add_systems for the same function signature as App::add_systems?

This is a nice pattern, you can use # to hide the distracting setup lines.

If so, should I obfuscate the naming and pretend that it's an app? It would look the same as App from an outside perspective, but the actual implementation would use ScheduleLabels with a dummy Update label.

Yeah :)

[IRSMsoso]

Now the examples look indistinguishable from normal app code with the exception of many instances of app.run(&mut world). This can't be hidden as many of the examples show the difference between multiple runs of systems with the conditions they are documenting. App::run typically takes no parameters, however, so it still has the potential to be confusing. Not sure how to get around this. The PR does now fix the original issue. The example in the rendered doc for the on_message condition now looks like:

Example

app.add_systems(
    Update,
    my_system.run_if(on_message::<MyMessage>),
);

#[derive(Message)]
struct MyMessage;

fn my_system(mut counter: ResMut<Counter>) {
    counter.0 += 1;
}

// No new `MyMessage` messages have been pushed so `my_system` won't run
app.run(&mut world);
assert_eq!(world.resource::<Counter>().0, 0);

world.resource_mut::<Messages<MyMessage>>().write(MyMessage);

// A `MyMessage` message has been pushed so `my_system` will run
app.run(&mut world);
assert_eq!(world.resource::<Counter>().0, 1);

I personally think it would be least confusing to just keep the type as Schedule and rename the variable to schedule to indicate that it's not actually an App, but if how the PR is now is preferable, then I believe it's ready to review.

[alice-i-cecile]

I personally think it would be least confusing to just keep the type as Schedule and rename the variable to schedule to indicate that it's not actually an App

Yeah, now that I can see the actual diff I think you're right 😅 Sorry for making you run around!

[IRSMsoso]

All good :) Should be ready for review.

[hymm]

I personally think it would be least confusing to just keep the type as Schedule and rename the variable to schedule to indicate that it's not actually an App, but if how the PR is now is preferable, then I believe it's ready to review.

My issue with this is that people will copy the code and it won't work for them. If they understand what's going on they'll just use the App api, but I worry that a fair number of people might try and get the schedule with the resource apis or not know how to fix it at all.

It's at worst just a minor issue, so I'm fine with the pr as is.

[IRSMsoso]

Yeah, I think best case scenario is something like depending on bevy_app only for doctests (not sure if that’s possible though). But in my eyes, it’s more important for the doc to be truthful than for it to be easily copy-pasteable. Part of the greatness of doctests is that it is easy to detect when they stop telling the truth about API surface because they will stop compiling. Mocking App means that it won’t necessarily have that ability anymore.

When I look at these docs as a user, I can see myself trying to use add_systems without the Schedule param, failing, then thinking to myself “hmm, it must be wrong because it’s called schedule, I need to investigate further”. But if it’s named app and it fails (either because we couldn’t fully mock it due to technical reasons or because the real App API changed and the mock wasn’t updated to match due to the mock still compiling), then I can see myself as a user not only being more confused (it’s called app it should work), but I can also see myself losing trust in the doc examples. “I have to look closer into the examples for the truth because sometimes it’s not how it appears”.

But I agree, it’s a pretty small issue either way.