allwpilib
allwpilib copied to clipboard
Make usage of `Set` and `...` syntax more consistent
In many places (eg. Commands.java) things like Set<Command> and Set<Subsystem> are used inconsistently with Command... commands or Subsystem... subsystems. This should be fixed either by adding more overloads so as to support both or by removing some and adding others.
One case I'm assuming you're referring to is Defer, which uses Set<Subsystem> so that not passing any requirements is explicit.
addRequirements has overloads for both, especially as getRequirements returns a Set.
What others are there?
The cases I could find:
CommandScheduler.cancel(Command...)- Having aCollection<Command>overload would probably be fine, but there isn't a clear use-case for canceling aCollection<Command>, and in general we avoid adding stuff that nobody would use.CommandScheduler.isScheduled(Command...)- I honestly don't know when this method is called with more than one argument, let alone with a collection.CommandScheduler.registerComposedCommands(Command...)- ACollection<Command>overload would be useful forSelectCommand, though it wouldn't have any other uses. (I'd probably make aSet<Command>overload that skips the check for duplicates, and have theCommand...andCollection<Command>overloads just do the duplicate checks)CommandScheduler.registerSubsystem(Subsystem...)- I honestly don't know when this method is called with more than one argument, let alone with a collection. (The name is singular, too)CommandScheduler.schedule(Command...)- We discourage manually callingschedule()in user code, and the only times I saw a single call being used to schedule multiple commands at a time, it was with varargs, not an already existing array object. (RobotDisabledCommandTest.java#L124, RobotDisabledCommandTest.java#L148, and CommandScheduleTest#L71)CommandScheduler.unregisterSubsystem(Subsystem...)- I honestly don't know when this method is called with more than one argument, let alone with a collection. (The name is singular, too)Command.addRequirements(Subsystem...)- This is covered in #6304- Command decorators, factories and constructors that use multiple commands (
Command.alongWith(Command...),Command.andThen(Command...),Command.deadlineFor(Command...),Command.deadlineWith(Command...)(deprecated),Command.raceWith(Command...),Commands.deadline(Command, Command...),Commands.parallel(Command...),Commands.race(Command...),Commands.repeatingSequence(Command...),Commands.sequence(Command...),ParallelCommandGroup(Command...),ParallelDeadlineGroup(Command, Command...),ParallelRaceGroup(Command...),ScheduleCommand(Command...),SequentialCommandGroup(Command...),SequentialCommandGroup.addCommands(Command...)) I'm ambivalent with these- I don't know of any cases where having aCollection<Command>overload would be useful, but someone might be able to provide an example. - Command decorators, factories and constructors that use requirements (
Command.andThen(Runnable, Subsystem...),Command.beforeStarting(Runnable, Subsystem...),Commands.idle(Subsystem...),Commands.run(Runnable, Subsystem...),Commands.runEnd(Runnable, Runnable, Subsystem...),Commands.runOnce(Runnable, Subsystem...),Commands.startEnd(Runnable, Runnable, Subsystem...),Commands.startRun(Runnable, Runnable, Subsystem...),FunctionalCommand(Runnable, Runnable, Consumer<Boolean>, BooleanSupplier, Subsystem...),InstantCommand(Runnable, Subsystem...),MecanumControlCommand(...)(4 overloads),NotifierCommand(Runnable, double, Subsystem...),PIDCommand(...)(2 overloads),ProfiledPIDCommand(...)(4 overloads),RamseteCommand(...)(2 overloads, deprecated),RunCommand(Runnable, Subsystem...),StartEndCommand(Runnable, Runnable, Subsystem...),SwerveControllerCommand(...)(4 overloads),TrapezoidProfileCommand(...)(1 overload)) I could see the case for making overloads that take aSet<Subsystem>for the requirements.
Given how trivial it is to convert between the two using Set.of/Set.toArray, I don't think we should duplicate everything for rare cases.
Can this be closed?