bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add missing schedules in `add_systems` doc (#11814)

Open Purfakt opened this issue 1 year ago • 8 comments
trafficstars

Objective

Fixes (#11814)

Solution

I added Update where it was missing in the rustdoc.

Uncovered problem

The previous doc was mocking the app as @hymm said:

app doesn't actually exist in bevy_ecs. It's just mocked in the doc comments as a schedule.

Proposed solutions

  • @pablo-lua suggested to just rename app in schedule and let the example be correct without the changes of the PR as it is, in fact, a schedule

  • @hymm suggested to move the add_systems impl on App onto World and just use that on the impl on App

Purfakt avatar Feb 10 '24 18:02 Purfakt

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

github-actions[bot] avatar Feb 10 '24 18:02 github-actions[bot]

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

Purfakt avatar Feb 10 '24 18:02 Purfakt

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.

hymm avatar Feb 10 '24 18:02 hymm

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) An possible option is changing the var name from app to something else

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

pablo-lua avatar Feb 10 '24 18:02 pablo-lua

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.

hymm avatar Feb 10 '24 18:02 hymm

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!

Purfakt avatar Feb 10 '24 19:02 Purfakt

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.

pablo-lua avatar Feb 10 '24 19:02 pablo-lua

I wouldn't mind having this pr change the examples in bevy_ecs to use schedule, so at least they're correct. We can make an issue for a follow up to discus adding a add_systems method to world.

hymm avatar Feb 11 '24 19:02 hymm