-
Notifications
You must be signed in to change notification settings - Fork 681
[cmd] Add Command.repeatedly(int count) #6589
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
base: main
Are you sure you want to change the base?
Conversation
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Show resolved
Hide resolved
spacey-sooty
left a comment
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.
Overall looks good just a few comments. Dunno why C++ would print stuff out especially since Java doesn't, along with this for printing in C++ its generally better to use fmt::print()
wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/native/cpp/frc2/command/CommandPtr.cpp
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp
Outdated
Show resolved
Hide resolved
|
/format |
|
This was stated in Discord, but tests are probably failing because of an inconsistent order of when the commands in the internal deadline group are checked, which should be fixed by #6602. |
c21b4cf to
832d5a0
Compare
|
Thanks to #6602 being merged, the tests pass correctly. I reverted back the print code and rebased them out for a cleaner history. |
KangarooKoala
left a comment
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.
Nice! Overall looks good, just a few small things.
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/CommandDecoratorTest.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/test/native/cpp/frc2/command/CommandDecoratorTest.cpp
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/native/cpp/frc2/command/CommandPtr.cpp
Outdated
Show resolved
Hide resolved
832d5a0 to
e122b0c
Compare
|
fix formatting with rebase + force push: |
f0d8608 to
b569c8f
Compare
KangarooKoala
left a comment
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 to me, for what it's worth.
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/CommandDecoratorTest.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/native/include/frc2/command/Command.h
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/native/include/frc2/command/CommandPtr.h
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/test/native/cpp/frc2/command/CommandDecoratorTest.cpp
Outdated
Show resolved
Hide resolved
|
/format |
3a36e0d to
cd2328e
Compare
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Outdated
Show resolved
Hide resolved
| * Decorates this command to run repeatedly until the given count is reached | ||
| * or is interrupted. The decorated command can still be canceled. |
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.
We should either make Java match C++ or C++ match Java. (I don't have a particularly strong preference on which one)
This also applies to the CommandPtr doc comment.
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 would mean we would neet to also change the documentation on the .repeating() function.
IMO, it would be better if this PR didn't have any line removals
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 would mean we would neet to also change the documentation on the
.repeating()function.
How so? I'm just talking about changing the repeatedly(int) doc comments.
Also, the wording that would most match the repeatedly() doc comment (in both Java and C++) is "Decorates this command to run repeatedly, restarting it when it ends, until this command is run the specified number of times or is interrupted. The decorated command can still be canceled."
57ceb5b to
7be3682
Compare
Co-authored-by: Joseph Eng <[email protected]>
7be3682 to
f31b58c
Compare
wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Command.java
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/test/native/cpp/frc2/command/CommandDecoratorTest.cpp
Show resolved
Hide resolved
KangarooKoala
left a comment
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.
Great work! There's just a few things left:
- The Java and C++ doc comments still need to be updated (see this comment).
- Pick a consistent name for the parameter (repetitions or counts) and make sure the parameter doc comments are the same between Java and C++.
wpilibNewCommands/src/test/native/cpp/frc2/command/CommandDecoratorTest.cpp
Outdated
Show resolved
Hide resolved
Also adds Java Consistency
|
/format |
KangarooKoala
left a comment
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.
Great work! Unfortunately the last 10% of polishing takes a while, but you're almost there.
wpilibNewCommands/src/main/native/include/frc2/command/CommandPtr.h
Outdated
Show resolved
Hide resolved
wpilibNewCommands/src/main/native/include/frc2/command/CommandPtr.h
Outdated
Show resolved
Hide resolved
Co-Authored-By: Joseph Eng <[email protected]>
c1c346a to
e6b7f42
Compare
That sure took a while, alright. Sorry, I kinda left the PRs just abandoned for a bit, life got a bit in the way |
Currently, the unit tests fail and there are a lot of print statements for whoever might be checking into this.
The problem is. The exact same function calls create different results between C++ and Java. I may just be the idiot but it seems to require further look into
The debug's are in different commits making it a lot easier to revert them if needed.