allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

Make usage of `Set` and `...` syntax more consistent

Open spacey-sooty opened this issue 1 year ago • 3 comments

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.

spacey-sooty avatar May 21 '24 13:05 spacey-sooty

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?

Starlight220 avatar May 21 '24 15:05 Starlight220

The cases I could find:

  • CommandScheduler.cancel(Command...)- Having a Collection<Command> overload would probably be fine, but there isn't a clear use-case for canceling a Collection<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...)- A Collection<Command> overload would be useful for SelectCommand, though it wouldn't have any other uses. (I'd probably make a Set<Command> overload that skips the check for duplicates, and have the Command... and Collection<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 calling schedule() 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 a Collection<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 a Set<Subsystem> for the requirements.

KangarooKoala avatar May 21 '24 20:05 KangarooKoala

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.

Starlight220 avatar May 22 '24 05:05 Starlight220

Can this be closed?

rzblue avatar Jul 10 '24 18:07 rzblue