Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change system set builder to force users to put the "config" first #1994

Closed
hymm opened this issue Apr 23, 2021 · 3 comments
Closed

Change system set builder to force users to put the "config" first #1994

hymm opened this issue Apr 23, 2021 · 3 comments
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@hymm
Copy link
Contributor

hymm commented Apr 23, 2021

What solution would you like?

with_system on the SystemSet builder returns something that only implements with_system, so config things like labeling, run_criteria, etc. must be placed before the systems.

What problem does this solve or what need does it fill?

This would fix #1912.

This would explicitly enforce @cart's preference on this comment #1909 (comment)

What alternative(s) have you considered?

We can just keep things as they are. Cargo fmt disambiguates this decently.

@hymm hymm added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Apr 23, 2021
@Ratysz
Copy link
Contributor

Ratysz commented Apr 23, 2021

I prefer this style as well, but outright forbidding alternatives seems needlessly restrictive. Not to mention the small but non-negligible API complication that would be necessary.

@Ratysz Ratysz added A-Core Common functionality for all bevy apps C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Apr 23, 2021
@cart
Copy link
Member

cart commented Apr 23, 2021

Agreed. Adopting the "type state" approach here would over-complicate the api and adopting an internal "state tracker" also feels pretty messy.

I'd rather just encourage it as a "best practice" in official examples.

@cart cart closed this as completed Apr 23, 2021
@cart
Copy link
Member

cart commented Apr 23, 2021

(feel free to continue discussing if you want. I'll reopen if any major dissenting opinions show up)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

3 participants