allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

Command factory methods for pre-built commands

Open shueja opened this issue 3 years ago • 6 comments

Inspired by the sequence(), parallel(), etc convenience methods from CommandGroupBase...

It would be nice if WPILib provided static factory methods for common uses of the pre-built commands. Many of these just call corresponding constructors to reduce the clutter of new keywords. Examples: The following could be moved from CommandGroupBase: sequence(), parallel(), race(), deadline() The next several have clear correspondences. wait(), waitUntil(), instant(), run(), startEnd(), runEnd(), ~~functional(), perpetually(), repeat()~~ A special one: none() -> new InstantCommand() This is useful for empty branches of conditional commands.

I expect that these factories don't overlap too much with the role of the decorators, but instead can be used in tandem. Any I missed?

shueja avatar May 15 '22 01:05 shueja

  • I don't think functional is a good idea, there are too many parameters for the verbosity decrease to be significant.
  • repeat and endless should be decorators and not factories; in general I think that anything that a decorator is more expressive for anything with a single command parameter.
  • how useful would startEnd/runEnd be? If keeping them, I'd also add startUntil and runUntil.

Starlight220 avatar May 15 '22 06:05 Starlight220

Points 1 and 2 make sense. I was just going down the list of command types tbh.

As far as startEnd and runEnd, I can say that most of the base level command factories in my subsystems are RunEndCommands. Ex. return new RunEndCommand(this::spin, this::stop, this); vs return runEnd(this::spin, this::stop, this); I don't use StartEndCommand as much, mostly because I don't often have a need for a "do nothing" command bookended by two Runnables.

Perhaps there was some miscommunication, but I don't see the extra value in runUntil(this::doThing, condition, this) vs run(this::doThing, this).until(condition), the latter in fact having IMO a better semantic ordering of things, "Run this method, require this subsystem, keep going until this happens".

shueja avatar May 15 '22 06:05 shueja

As for your first point, I agree that RunEndCommand would be useful, though I have seen usages of StartEndCommand (mostly for toggle operations, but there were other cases).

As for your second point, the only advantage it would probably give is less nesting and some semantics, though in the same manner, one could say that runEnd could be better put as run().until().andThen() (though I guess with interruption it gets a bit weird). It's not a hill I intend to die on.

Starlight220 avatar May 15 '22 10:05 Starlight220

Should C++ get a generic CreateCommand method to sugar the make_unique call? (#4303)

Starlight220 avatar Jun 16 '22 19:06 Starlight220

I don't think CreateCommand<InstantCommand> is any better than std::make_unique<InstantCommand>. A better sugar would be e.g. InstantCommand::Create(...) instead of std::make_unique<InstantCommand>(...).

PeterJohnson avatar Jun 17 '22 17:06 PeterJohnson

The question is how to add that automagically to user-defined command classes? Does C++ inheritance support static members? Or perhaps we can mimic the Kotlin pseudoconstructor idiom, overloading the () operator on the type and returning a unique_ptr directly? (That would require removing/making private the regular ctor due to ambiguity)

Orthogonally, I think that teams (especially lower-experience teams) would be more comfortable invoking a WPILib function than an stdlib function -- we talked about this in the meeting.

Starlight220 avatar Jun 18 '22 19:06 Starlight220