-
Notifications
You must be signed in to change notification settings - Fork 611
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
[cmd] Add missing C++ decorators #6599
[cmd] Add missing C++ decorators #6599
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.
This PR does a lot -- maybe split to one that adds the missing decorators in C++ and another that preserves and tests execution order?
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/CommandDecoratorTest.java
Show resolved
Hide resolved
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/CommandDecoratorTest.java
Show resolved
Hide resolved
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.
Looks good overall
|
||
Command command = new WaitCommand(10).until(condition::get); | ||
Command command = new RunCommand(() -> {}).until(finish::get); |
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.
All these instances of RunCommand(() -> {})
can be replaced with Commands.idle()
.
Not important enough to block this PR, though.
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.
I opened #6652 since there's a bunch of other spots in command tests that should also be updated. If this PR gets merged before the issue gets resolved, I'll update the list on the issue, and if the issue gets resolved first, then I'll update this PR.
Notes:
CommandPtr
didn't already haveAndThen
,DeadlineFor
,AlongWith
, andRaceWith
decorators that take multiple commands, so I didn't add equivalents of those toCommand
. (I did add the single command argument versions, though) This unfortunately affected theDeadlineFor
test, so if people want me to add those as well in this PR then I'm fine with doing that. (Or if they want to do it themselves in a separate PR, that's cool too)Until
,OnlyWhile
,DeadlineFor
,AlongWith
,RaceWith
, where the textual order of arguments matches the order of processing. (The decorated command is processed first (execute()
run andisFinished()
checked, optionally runningend()
), then the arguments to the decorator)