Added withDeadline modifier #7299
Conversation
|
This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR. |
Pretty sure it is |
It can't be done in C++ due to the opaqueness of the underlying command type. Specific return types would cause object slicing issues when used (that's why CommandPtr was introduced). Now onto Java: The return type opaqueness is intentional, to allow for encapsulation and any later implementation changes in a non-breaking way. There's little-to-no advantage to specifying exact command types, and it breaks on any changes. The added method itself doesn't add anything, imo. It doesn't use the fact that it's a parallel group. It's more verbose than the alternatives. |
|
I doubt that the implementation of Commands.parallel() will change in the future; at most, any overhauls to parallel groups will modify the existing classes and not new ones |
|
My personal take- I agree that there's significant value in distinguishing the deadline command from the other commands. However, I also agree that it's good library design to keep the return types of the factories as an opaque These two statements lead me to the idea: Add a @Starlight220 @Daniel1464 Thoughts? |
|
I think that probably would be a better implementation of my current idea. |
|
/format |
|
Re I guess go for it? |
| /** | ||
| * Creates a new command that runs this command and the deadline in parallel, finishing when the | ||
| * deadline finishes. | ||
| * | ||
| * @param deadline the deadline of the command group | ||
| * @return the decorated command | ||
| */ |
There was a problem hiding this comment.
This should include the standard composition warning. (You can copy-paste from the neighboring methods).
There was a problem hiding this comment.
These changes should be reverted.
wpilibNewCommands/src/main/native/cpp/frc2/command/CommandPtr.cpp
Outdated
Show resolved
Hide resolved
|
/format |
hal/generate_usage_reporting.py
Outdated
| @@ -3,7 +3,6 @@ | |||
| # Copyright (c) FIRST and other WPILib contributors. | |||
| # Open Source Software; you can modify and/or share it under the terms of | |||
| # the WPILib BSD license file in the root directory of this project. | |||
|
|
|||
There was a problem hiding this comment.
i believe it was something that came up when attempting to merge from main
There was a problem hiding this comment.
Seems to have been a weird interaction between the merge and the formatting. Just manually add the line back to clean up the PR diff.
| /** | ||
| * Creates a new command that runs this command | ||
| * and the deadline in parallel, finishing when | ||
| * the deadline finishes. | ||
| * | ||
| * @param deadline the deadline of the command group | ||
| * @return the decorated command | ||
| */ |
There was a problem hiding this comment.
Open question: Do we want to include an explicit note that the calling command will be interrupted when the other command finishes? (This would make it match more with DeadlineFor)
Also, do we want to add @see DeadlineFor to the WithDeadline() doc comment and @see WithDeadline to the DeadlineFor doc comment?
(Both of these apply to Java, by the way)
There was a problem hiding this comment.
makes sense; i'll get around to it soon
Currently, a lot of teams use the Commands.parallel(), Commands.race() and Commands.deadline() static factory methods for parallel groups. However, Commands.deadline() doesn't communicate its purpose very well(it doesn't mention the word "parallel" at all, and it doesn't make clear what the deadline of the parallel group is). Even though the deadlineWith(soon to be called deadlineFor) modifier exists, it introduces inconsistency in syntax if a team uses the parallel() and race() factories(as it is an instance method).
Thus, a withDeadline() modifier is added to the ParallelCommandGroup class to allow for
Commands.parallel(a,b,c).withDeadline(d). This solves the issues of syntax inconsistency while expressing the intent better to code readers. In addition, the parallel factory methods now have a return type of their respective class, which allows for this syntax to exist.