Skip to content
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

Closed
wants to merge 0 commits into from

Conversation

katzuv
Copy link
Contributor

@katzuv katzuv commented Oct 11, 2024

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.

@katzuv katzuv requested a review from a team as a code owner October 11, 2024 12:57
Copy link
Contributor

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public SequentialCommandGroup thenAwaitTimeout(double seconds) {
public SequentialCommandGroup thenTimeout(double seconds) {

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thenWait?

Copy link
Contributor Author

@katzuv katzuv Oct 11, 2024

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

Copy link
Contributor Author

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.

Comment on lines 250 to 251
public SequentialCommandGroup thenAwaitTimeout(Time time) {
return thenAwaitTimeout(time.in(Seconds));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Command awaitTimeout = new RunCommand(() -> {}).thenAwaitTimeout(0.1);
Command awaitTimeout = new RunCommand(() -> {}).thenTimeout(0.1);

@katzuv
Copy link
Contributor Author

katzuv commented Oct 11, 2024

thenAwait or andThenAwait?

@katzuv katzuv changed the title [cmd] Add thenAwait and thenAwaitTimeout command decorators [cmd] Add thenAwait and thenWait command decorators Oct 13, 2024
@katzuv
Copy link
Contributor Author

katzuv commented Oct 13, 2024

/format

@Starlight220
Copy link
Member

Maybe (and)ThenWaitUntil()?

@katzuv
Copy link
Contributor Author

katzuv commented Oct 13, 2024

Maybe (and)ThenWaitUntil()?

I think "await" == "wait until", no?
The "and" can be added, I don't know what the preferences are for more than two words for decorators.

@spacey-sooty
Copy link
Contributor

/format

The format command won't work cause the PR is from the main branch of your form

@katzuv
Copy link
Contributor Author

katzuv commented Oct 13, 2024

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 (then-wait-await) but GitHub lets me change the destination branch only. Can someone with higher permissions edit the PR? Otherwise I'll open a new one, if the formatting is incorrect.

@spacey-sooty
Copy link
Contributor

Java format is failing, locally you can just run ./gradlew spotlessApply which should fix it

@KangarooKoala
Copy link
Contributor

Personally I'd like these to be (and)Then... followed by the named of the Commands factory (e.g., thenWaitUntil(BooleanSupplier) or andThenWaitUntil(BooleanSupplier)), since that should make it easier for people to remember.

@katzuv katzuv requested review from PeterJohnson and a team as code owners October 14, 2024 13:14
@katzuv katzuv changed the title [cmd] Add thenAwait and thenWait command decorators [cmd] Add andThenWaitUntil, andThenWaitSeconds, andThenWaitTime command decorators Oct 14, 2024
@rzblue
Copy link
Member

rzblue commented Oct 14, 2024

I'm not a fan of adding these.

andThenWaitUntil(foo())
andThen(waitUntil(foo()))

andThenWaitSeconds(foo())
andThen(waitSeconds(foo()))

You save two characters.

@katzuv
Copy link
Contributor Author

katzuv commented Oct 14, 2024

I'm not a fan of adding these.

andThenWaitUntil(foo())
andThen(waitUntil(foo()))

andThenWaitSeconds(foo())
andThen(waitSeconds(foo()))

You save two characters.

True. I did imagine them more concise, with it initially being thenAwait. But it does save you the Commands. part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants