Python generator-based impl #24
Conversation
tests/CommandTestBase.py
Outdated
| self.value = value | ||
|
|
||
|
|
||
| class CommandTestBase(unittest.TestCase): |
There was a problem hiding this comment.
Is there a reason you're using python unittest library? pytest is much less verbose and is preferred (and is what our other tests use).
There was a problem hiding this comment.
Nope, this is what IntelliJ gave me and I didn't know better to change.
I'll change to use pytest.
|
|
||
|
|
||
| class Command: | ||
| requirements: Set[Requirement] = {} |
There was a problem hiding this comment.
Should use instance variables, not class variables.
There was a problem hiding this comment.
The types don't match in this statement either. The RHS is a dict.
| from .requirement import Requirement, InterruptBehavior | ||
|
|
||
|
|
||
| class Command: |
There was a problem hiding this comment.
I see this has one abstract method.
I think this whole class should be a Protocol then.
@auscompgeek Thoughts on that? Protocol vs ABC?
There was a problem hiding this comment.
Depends on when we want errors to be caught I guess. If we want errors when instantiating the class then an ABC is appropriate. Otherwise if we want duck typing then a Protocol will provide that.
If there's only one method, and no custom constructor, probably no huge advantage to an ABC?
| setattr(self, "_composed", True) | ||
|
|
||
|
|
||
| def commandify( |
There was a problem hiding this comment.
If you really do want to shoehorn end into the coroutine version, you have have commandify return return a class that can be used as a decorator to specify end.
@commandify
def my_command():
...
@my_command.end
def the_end_for_my_command():
...I'd rather omit end and only support it for class based commands
There was a problem hiding this comment.
I think we could support end by throwing an exception into the coroutine
There was a problem hiding this comment.
Yeah I think that would be better. There could be a CommandInterrupted exception.
There was a problem hiding this comment.
I think that would complicate user-side code.
Honestly, I kindof like the .end decorator idea.
There was a problem hiding this comment.
Well I think ideally most users don't use end at all and this is just a workaround for the few that do. By making it an exception, you have access to the existing context of the coroutine.
|
Two more thoughts:
|
| assert cond.getCondition() | ||
| assert not scheduler.isScheduled(cmd) | ||
| assert cond.get_condition() | ||
| assert not scheduler.is_scheduled(cmd) |
There was a problem hiding this comment.
If we were to provide a python implementation of commands, a primary goal would be to attempt to keep backwards compat with the current commands so that teams don't have to rewrite everything next year. Historically, we've used java camelcase conventions for function names, so we should stick to that.
Ideally, all of this year's tests (and any tests written for this effort that aren't coroutine related) should pass without modification on either the C++ codebase or the pure python implementation.
I actually preserved that functionality in sphinxify as well |
No description provided.