-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Initialize empty schedules when calling .in_schedule
if they do not already exist
#7911
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
This is not correct; you're creating a |
My mistake in PR description. Fixed it. |
also, maybe same changes might be applied in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior should be documented in the doc comments.
Yes, definitely. And agreed with James about the doc comments: I think |
Isn't this part covered with some tests that also need to be updated? I can't find any... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I think this is a good change. In addition to the others' feedback, I have a bit of my own.
Also, please run cargo fmt --all
. That should fix the CI issues :)
I'm not surprised if there aren't any: we tend not to test this sort of end-user API super hard since it gets caught readily in the examples. That said, I'd be happy to see a pair of tests for this behavior, one for the single system case and the other for multiple systems. |
how can I test that system runs on a specified schedule? Like in this example, to test that it runs not on default? |
Use |
should these tests be integration tests, in |
The last one will work great here :) |
maybe for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct now. It's probably also worth mentioning in the docs for add_state
that the OnEnter
and OnExit
schedules only get added if they don't already exist.
.in_schedule
if they do not already exist
Objective
Fixes #7910
Solution
create shedule if it does not exist by the time
add_system
calledMy first PR. Not sure how much this solution feet here but at least it seems to work. Any comment is appreciated.