Add missing schedules in add_systems doc (#11814) - #11815
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
|
I'm not sure I understand why, when I try to run the rustdoc, I get: error[E0425]: cannot find value `Update` in this scope
--> crates/bevy_ecs/src/system/combinator.rs:57:5
|
40 | Update,
| ^^^^^^ not found in this scope
error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> crates/bevy_ecs/src/system/combinator.rs:56:5
|
39 | app.add_systems(
| ^^^^^^^^^^^
40 | Update,
| ____________-
41 | |/ my_system.run_if(Xor::new(
42 | || IntoSystem::into_system(resource_equals(A(1))),
43 | || IntoSystem::into_system(resource_equals(B(1))),
44 | || // The name of the combined system.
45 | || std::borrow::Cow::Borrowed("a ^ b"),
46 | || )));
| || -
| ||__|
| |__help: remove the extra argument
| unexpected argument of type `NodeConfigs<Box<(dyn bevy_ecs::system::System<In = (), Out = ()> + 'static)>>`
|
note: method defined here
--> /home/purfakt/dev/perso/bevy/crates/bevy_ecs/src/schedule/schedule.rs:249:12
|
249 | pub fn add_systems<M>(&mut self, systems: impl IntoSystemConfigs<M>) -> &mut Self {
| ^^^^^^^^^^^
error: aborting due to 2 previous errors |
|
This is because app doesn't actually exist in bevy_ecs. It's just mocked in the doc comments as a schedule. Not sure how to fix this. |
|
I think this can't be solved unless we import bevy_app into bevy_ecs (that would probably defeat the purpose of bevy_ecs being independent) Here we have the definition of the var: /// # let mut app = Schedule::default();
We would first remove the # to clear that we are not talking about the App here and rename app to schedule /// let mut schedule = Schedule::default();
That would clear up the problem, I think |
|
We could move the add_systems impl on App onto world and just use that on the impl on App. More code duplication, but might be worth it. |
|
Oh I see. I'm not sure if I'm capable or entitled to make this change. I didn't expect my first PR to be tagged controversial. Especially, when it was just a documentation update! |
Well, this means that the issue is rather good, as this shows that there is a misunderstood going about what is the app that the documentation is talking about, a third opinion might be good here in regarding of what to do for sure. |
|
I wouldn't mind having this pr change the examples in |
|
Seems like there's agreement about renaming the variable to |
# 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 bevyengine#11814 Alternative to bevyengine#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. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
|
This PR can probably be closed now |
Objective
Fixes (#11814)
Solution
I added
Updatewhere it was missing in the rustdoc.Uncovered problem
The previous doc was mocking the app as @hymm said:
Proposed solutions
@pablo-lua suggested to just rename
appinscheduleand let the example be correct without the changes of the PR as it is, in fact, aschedule@hymm suggested to move the
add_systemsimpl onAppontoWorldand just use that on the impl onApp