Closed banditopazzo closed 2 months ago
Hello, I hope I'm not barging in :sweat_smile: . Posting here since Domenico is OOO.
I've been talking with Domenico this week about the interfaces exposed here for PulsarModule
and Extra<T: PulsarModule>
as he was finding the relationship between Extra
and PulsarModule
hacky and non-intuitive.
Quite a few unsatisfactory ideas later I still had a background thread thinking about this and I'm actually embarrassed for not figuring this out much sooner, but the desired behavior that Domenico wants can be achieved this way: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6dd15c4e5d7b2a84225ef7aeaad4f5cc
Just have the PulsarModule
be a combination of the current PulsarModule
and Extra
. Users that want the "full" behavior (with trigger
and action
) implement PulsarModule
.
For "basic" modules, expose another trait: BasicPulsarModule
, which is a copy of PulsarModule
except for the action
and trigger
methods which are missing. Then do a blanket impl<T: BasicPulsarModule> PulsarModule for T
which uses the "dummy" action and trigger methods that the current NoExtra
implements. Users can choose to implement this trait instead if they don't need the additional functionalities that PulsarModule
provides. And then just use PulsarModule
in trait bounds, as both implementors of PulsarModule
and BasicPulsarModule
will work.
With some basic documentation explaining the difference between PulsarModule
and BasicPulsarModule
people should not have problems choosing which one to use. I think this conveys better what each of the interfaces are for, can be documented properly, and especially removes the need to set the confusing Extra
associated type to some dummy NoExtra
type.
@vadorovsky I could make these changes myself and push a commit if you want this done faster while Domenico is away.
@bobozaur Sorry for missing your message! How long is Domenico OOO?
Yes, I like the BasicPulsarModule
vs PulsarModule
distinction much more than PulsarModule
and Extra
. Your example looks good to me too. I would say, feel free to push the changes or make a separate PR.
@vadorovsky I opened a separate PR. The calendar entry says Domenico will be out until Wednesday, which is why I thought it might be better to leave my suggestion here in case there's any urgency.
Ideally I'd prefer to wait for Domenico to get back before merging my PR just to make sure he's on board with the approach as well, but I'll leave it to you if you think this should be merged sooner.
Add support for an extra function operating on the state.
The function needs a
trigger
(aFuture
) and anaction
(anotherFuture
). Under the hood they are used in atokio::select!
in the main loop of a module similar to this: