From 8aeee03626725bdba15538d547f196ebf9bb6365 Mon Sep 17 00:00:00 2001 From: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com> Date: Tue, 26 Dec 2023 18:07:16 -0800 Subject: [PATCH] [commands] Improve error message when composing commands twice in same composition (#6091) Also disallow deadline command from also appearing in other commands. --- .../wpilibj2/command/CommandScheduler.java | 13 +++++++-- .../wpi/first/wpilibj2/command/Commands.java | 7 +++-- .../command/ParallelDeadlineGroup.java | 28 +++++++++++-------- .../command/MultiCompositionTestBase.java | 16 +++++++++++ .../command/ParallelDeadlineGroupTest.java | 16 +++++++++++ 5 files changed, 64 insertions(+), 16 deletions(-) diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java index 4c87c66e25..e61b6f8880 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java @@ -581,10 +581,19 @@ public final class CommandScheduler implements Sendable, AutoCloseable { * directly or added to a composition. * * @param commands the commands to register - * @throws IllegalArgumentException if the given commands have already been composed. + * @throws IllegalArgumentException if the given commands have already been composed, or the array + * of commands has duplicates. */ public void registerComposedCommands(Command... commands) { - var commandSet = Set.of(commands); + Set commandSet; + try { + commandSet = Set.of(commands); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + "Cannot compose a command twice in the same composition! (Original exception: " + + e + + ")"); + } requireNotComposedOrScheduled(commandSet); var exception = new Exception("Originally composed at:"); exception.fillInStackTrace(); diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Commands.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Commands.java index 7295e3cd43..9202d196a1 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Commands.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Commands.java @@ -233,12 +233,13 @@ public final class Commands { * the others. * * @param deadline the deadline command - * @param commands the commands to include + * @param otherCommands the other commands to include * @return the command group * @see ParallelDeadlineGroup + * @throws IllegalArgumentException if the deadline command is also in the otherCommands argument */ - public static Command deadline(Command deadline, Command... commands) { - return new ParallelDeadlineGroup(deadline, commands); + public static Command deadline(Command deadline, Command... otherCommands) { + return new ParallelDeadlineGroup(deadline, otherCommands); } private Commands() { diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroup.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroup.java index 6d263e88cf..0e6f110a56 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroup.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroup.java @@ -29,20 +29,18 @@ public class ParallelDeadlineGroup extends Command { private InterruptionBehavior m_interruptBehavior = InterruptionBehavior.kCancelIncoming; /** - * Creates a new ParallelDeadlineGroup. The given commands (including the deadline) will be + * Creates a new ParallelDeadlineGroup. The given commands, including the deadline, will be * executed simultaneously. The composition will finish when the deadline finishes, interrupting * all other still-running commands. If the composition is interrupted, only the commands still * running will be interrupted. * * @param deadline the command that determines when the composition ends - * @param commands the commands to be executed + * @param otherCommands the other commands to be executed + * @throws IllegalArgumentException if the deadline command is also in the otherCommands argument */ - public ParallelDeadlineGroup(Command deadline, Command... commands) { - m_deadline = deadline; - addCommands(commands); - if (!m_commands.containsKey(deadline)) { - addCommands(deadline); - } + public ParallelDeadlineGroup(Command deadline, Command... otherCommands) { + addCommands(otherCommands); + setDeadline(deadline); } /** @@ -50,11 +48,19 @@ public class ParallelDeadlineGroup extends Command { * contained. * * @param deadline the command that determines when the group ends + * @throws IllegalArgumentException if the deadline command is already in the composition */ public void setDeadline(Command deadline) { - if (!m_commands.containsKey(deadline)) { - addCommands(deadline); + @SuppressWarnings("PMD.CompareObjectsWithEquals") + boolean isAlreadyDeadline = deadline == m_deadline; + if (isAlreadyDeadline) { + return; } + if (m_commands.containsKey(deadline)) { + throw new IllegalArgumentException( + "The deadline command cannot also be in the other commands!"); + } + addCommands(deadline); m_deadline = deadline; } @@ -74,7 +80,7 @@ public class ParallelDeadlineGroup extends Command { for (Command command : commands) { if (!Collections.disjoint(command.getRequirements(), m_requirements)) { throw new IllegalArgumentException( - "Multiple commands in a parallel group cannot" + "require the same subsystems"); + "Multiple commands in a parallel group cannot require the same subsystems"); } m_commands.put(command, false); m_requirements.addAll(command.getRequirements()); diff --git a/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/MultiCompositionTestBase.java b/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/MultiCompositionTestBase.java index 46cb03cd9a..9fafc5905c 100644 --- a/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/MultiCompositionTestBase.java +++ b/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/MultiCompositionTestBase.java @@ -5,6 +5,7 @@ package edu.wpi.first.wpilibj2.command; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.params.provider.Arguments.arguments; import edu.wpi.first.wpilibj2.command.Command.InterruptionBehavior; @@ -112,4 +113,19 @@ abstract class MultiCompositionTestBase extends SingleComposi var command = compose(command1, command2, command3); assertEquals(expected, command.runsWhenDisabled()); } + + static Stream composeDuplicates() { + Command a = new InstantCommand(() -> {}); + Command b = new InstantCommand(() -> {}); + return Stream.of( + arguments("AA", new Command[] {a, a}), + arguments("ABA", new Command[] {a, b, a}), + arguments("BAA", new Command[] {b, a, a})); + } + + @MethodSource + @ParameterizedTest(name = "composeDuplicates[{index}]: {0}") + void composeDuplicates(@SuppressWarnings("unused") String name, Command[] commands) { + assertThrows(IllegalArgumentException.class, () -> compose(commands)); + } } diff --git a/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroupTest.java b/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroupTest.java index 789827bf6c..206d801015 100644 --- a/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroupTest.java +++ b/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/ParallelDeadlineGroupTest.java @@ -4,6 +4,7 @@ package edu.wpi.first.wpilibj2.command; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -124,6 +125,21 @@ class ParallelDeadlineGroupTest extends MultiCompositionTestBase new ParallelDeadlineGroup(command1, command2)); } + @Test + void parallelDeadlineSetDeadlineToDeadlineTest() { + Command a = new InstantCommand(() -> {}); + ParallelDeadlineGroup group = new ParallelDeadlineGroup(a); + assertDoesNotThrow(() -> group.setDeadline(a)); + } + + @Test + void parallelDeadlineSetDeadlineDuplicateTest() { + Command a = new InstantCommand(() -> {}); + Command b = new InstantCommand(() -> {}); + ParallelDeadlineGroup group = new ParallelDeadlineGroup(a, b); + assertThrows(IllegalArgumentException.class, () -> group.setDeadline(b)); + } + @Override public ParallelDeadlineGroup compose(Command... members) { return new ParallelDeadlineGroup(members[0], Arrays.copyOfRange(members, 1, members.length));