wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.06k stars 613 forks source link

Proxy static method in Commands #4853

Closed We-Gold closed 1 year ago

We-Gold commented 1 year ago

I'm currently using the ProxyCommand in multiple locations, and while making a class every time is not a big deal, it would be more convenient to have a proxy static method in the Commands class.

Additionally, having the Supplier<Command> overload would be helpful as well.

Starlight220 commented 1 year ago

making a class

What do you mean by this? ProxyCommand (and most other command types, in most situations) should be constructed inline, not subclassed. More on that here.

I'm not opposed, but I don't think there's benefit in adding a factory for the immediate-construction overload (the one accepting a Command) as we have the .asProxy() decorator.

For obvious reasons a decorator isn't possible for the delayed-construction overload (the Supplier<Command> one). We can add a factory function for it in Commands, though it doesn't save too much verbosity:

new ProxyCommand(supplier);
// vs
Commands.supplied(supplier);
We-Gold commented 1 year ago

I realize now I worded that strangely. I meant instantiating a ProxyCommand inline, like you demonstrated below.

I agree that the .asProxy() method makes it unnecessary to have a factory method. For the supplied one, I see it a bit differently. I'm importing the static methods at the top of the file since I call them frequently, so it would end up looking like supplied(supplier).

I'm not sure of the direction that commands are going in, but if it is towards inline with factory methods and decorators, it would make sense to have a factory method for the supplied proxy command as an alternative.

Starlight220 commented 1 year ago

For C++, ProxyCommand will need a constructor overload that accepts std::function<CommandPtr()>.

Gold856 commented 1 year ago

Since #5566 handles deferred commands, can this issue be closed?

Starlight220 commented 1 year ago

Linked that PR.