Skip to content

Add missing schedules in add_systems doc (#11814) - #11815

Open
Purfakt wants to merge 1 commit into
bevyengine:mainfrom
Purfakt:#11814-update-doc-missing-schedules
Open

Add missing schedules in add_systems doc (#11814)#11815
Purfakt wants to merge 1 commit into
bevyengine:mainfrom
Purfakt:#11814-update-doc-missing-schedules

Conversation

@Purfakt

@Purfakt Purfakt commented Feb 10, 2024

Copy link
Copy Markdown

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

@github-actions

Copy link
Copy Markdown
Contributor

Welcome, new contributor!

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

@Purfakt

Purfakt commented Feb 10, 2024

Copy link
Copy Markdown
Author

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

@hymm

hymm commented Feb 10, 2024

Copy link
Copy Markdown
Contributor

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.

@pablo-lua

pablo-lua commented Feb 10, 2024

Copy link
Copy Markdown
Contributor

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

@hymm

hymm commented Feb 10, 2024

Copy link
Copy Markdown
Contributor

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.

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events X-Needs-SME This type of work requires an SME to approve it. labels Feb 10, 2024
@Purfakt

Purfakt commented Feb 10, 2024

Copy link
Copy Markdown
Author

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!

@pablo-lua

Copy link
Copy Markdown
Contributor

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.

@hymm

hymm commented Feb 11, 2024

Copy link
Copy Markdown
Contributor

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.

@rparrett rparrett added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jan 22, 2025
@rparrett

Copy link
Copy Markdown
Contributor

Seems like there's agreement about renaming the variable to schedule.

@BenjaminBrienen BenjaminBrienen added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Jan 23, 2025
@cart cart closed this May 5, 2026
@cart cart reopened this May 5, 2026
beicause pushed a commit to beicause/bevy that referenced this pull request Jul 11, 2026
# 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>
@IRSMsoso

Copy link
Copy Markdown
Contributor

This PR can probably be closed now

@JaySpruce JaySpruce added S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 7, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in ECS Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. X-Needs-SME This type of work requires an SME to approve it.

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

9 participants