-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Remove systems from schedule #20298
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
Remove systems from schedule #20298
Conversation
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.
Exciting! Nice to see our schedule API getting more dynamic.
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
|
This fixes a long-enough standing issue that I'd like a release note. |
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.
The fundamental strategy seems good, and there's nice tests. Before we merge though:
- this fixes an important and old enough issue that I think this needs a release note
- I would like a few convenience APIs for this, ideally on World, App and Commands
- I would like a brief note in the docs comparing and contrasting this to run conditions
I am very excited about this work though; please let me know if you don't have time and I'll finish this up for you.
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.
Ooo, cool change!
There are a few doc changes I recommend to the primary functions. I also have some veeery optional docs for the tests:
Tests
Comments
I added some doc-comments to the tests. The other ones don't really do that, so feel free to leave em unchanged.
It's my preference for module-name-irrelevant stuff like this (but, tbh, it's nice for all tests lol)
World
It might be nice to have a quick schedule.run() test on the changes. Just to ensure things work right in practice aside from theory :)
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.
Ooh, this is exciting!
Co-authored-by: Barrett Ray <[email protected]>
9eb0470 to
07511f4
Compare
|
Updated the pr to have some removal policies, where we have options to
I'm not 100% convinced that a flat enum is the best choice here. It might be better to have some sort of configuration struct. Just brainstorming, but some other policies could be to remove sub sets, or run conditions. |
|
@chescock's suggestion is solid and we might as well clean this up now, but I'll merge this next Monday if you don't have time to get to that. |
|
I still need to update the release note |
Objective
Solution
remove_systems_in_setmethod which removes all the systems by their set name. In most cases systems are added to a schedule once, so just passing the system (which is an implicit set by it's type name) will just remove the one system. If a system is added multiple times such asapply_deferred, then you may need to give the system a unique set, if you just want to remove just one system.Testing
Future work