-
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 andThenWaitUntil
, andThenWaitSeconds
, andThenWaitTime
command decorators
#7184
Conversation
This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR. |
* @param seconds the timeout duration | ||
* @return the command with the timeout to await | ||
*/ | ||
public SequentialCommandGroup thenAwaitTimeout(double seconds) { |
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.
public SequentialCommandGroup thenAwaitTimeout(double seconds) { | |
public SequentialCommandGroup thenTimeout(double seconds) { |
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 think this might be a little less clear? Because you don't know what the timeout is there for.
But I think my choice of "timeout" is not very good, other suggestions are welcome
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.
thenWait
?
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.
Yes I think that's better! I'll change it tomorrow. Thank 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.
Actually I wonder if it's not too similar to thenAwait
.
public SequentialCommandGroup thenAwaitTimeout(Time time) { | ||
return thenAwaitTimeout(time.in(Seconds)); |
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.
public SequentialCommandGroup thenAwaitTimeout(Time time) { | |
return thenAwaitTimeout(time.in(Seconds)); | |
public SequentialCommandGroup thenTimeout(Time time) { | |
return thenTimeout(time.in(Seconds)); |
HAL.initialize(500, 0); | ||
SimHooks.pauseTiming(); | ||
try (CommandScheduler scheduler = new CommandScheduler()) { | ||
Command awaitTimeout = new RunCommand(() -> {}).thenAwaitTimeout(0.1); |
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.
Command awaitTimeout = new RunCommand(() -> {}).thenAwaitTimeout(0.1); | |
Command awaitTimeout = new RunCommand(() -> {}).thenTimeout(0.1); |
|
thenAwait
and thenAwaitTimeout
command decoratorsthenAwait
and thenWait
command decorators
/format |
Maybe |
I think "await" == "wait until", no? |
The format command won't work cause the PR is from the main branch of your form |
I created a new branch in my repo ( |
Java format is failing, locally you can just run ./gradlew spotlessApply which should fix it |
Personally I'd like these to be |
thenAwait
and thenWait
command decoratorsandThenWaitUntil
, andThenWaitSeconds
, andThenWaitTime
command decorators
I'm not a fan of adding these.
You save two characters. |
True. I did imagine them more concise, with it initially being |
I can add Python later and try to add C++ implementation. English is not my first language, so I'd appreciate suggestion for improving the docs.