rust-libp2p
rust-libp2p copied to clipboard
swarm/handler: Rename `InEvent` and `OutEvent` to `FromBehaviour` and `ToBehaviour`
Description
Rename the associated types of ConnectionHandler
from InEvent
to FromBehaviour
and OutEvent
to ToBehaviour
.
Motivation
Using a ConnectionHandler
outside of the NetworkBehaviour
abstraction is generally possible put very unlikely. To make it easier for users to understand, what these types do, we can rename them to not just be an "in" and "out" but to include where the event is going to be handled or where it is coming from.
Current Implementation
Are you planning to do it yourself in a pull request?
Maybe.
I find this more intuitive looking at ConnectionHandler
only. Though in the greater picture, including NetworkBehaviour
, it is inconsistent with NetworkBehaviour::OutEvent
.
I find this more intuitive looking at
ConnectionHandler
only. Though in the greater picture, includingNetworkBehaviour
, it is inconsistent withNetworkBehaviour::OutEvent
.
We can name that one ToSwarm
? :)
+1 in favor of using more concrete names for associated types.
Maybe we could instead of FromBehaviour
name InEvent
-> ToHandler
. If we then also follow @thomaseizinger proposal to rename NetworkBehaviour::OutEvent
to ToSwarm
we'd have consistend names ToHandler
, ToBehaviour
, ToSwarm
, etc.
Similar naming is also used in the relay v2 protocol for events between handler and listeners.
Related to this: What do you think about renaming NetworkBehaviour::inject_event
-> ::inject_handler_msg
and ConnectionHandler::inject_event
-> inject_behaviour_msg
(or something similar).
When first working with NetworkBehaviour
and Handler
as a user I struggled quite a bit with understanding how behaviour and handler are related and exchange messages. As @thomaseizinger wrote above, there is no real use-case for a ConnectionHandler
outside of a NetworkBehaviour
, so I think we can be a bit less abstract with our names here.
When first working with
NetworkBehaviour
andHandler
as a user I struggled quite a bit with understanding how behaviour and handler are related and exchange messages.
Same! Especially the ConnectionHandler
s OutEvent
is obviously the NetworkBehaviour
's InEvent
. Although that is inferred through the associated type, it is confusing because the NetworkBehaviour
has its own OutEvent
.
Related to this: What do you think about renaming
NetworkBehaviour::inject_event
->::inject_handler_msg
With https://github.com/libp2p/rust-libp2p/pull/2867/ NetworkBehaviour::inject_event
would move to NetworkBehaviour::on_event
via InEvent::ConnectionHandler
. (Where InEvent
would be renamed to FromSwarm
with this proposal I am guessing.) Would you be in favor of that @elenaf9?
and
ConnectionHandler::inject_event
->inject_behaviour_msg
(or something similar).
I suggest we do the same change on ConnectionHandler
, see also https://github.com/libp2p/rust-libp2p/issues/2832#issuecomment-1236252507.
I am in favor of the proposals in this issue.
Related to this: What do you think about renaming
NetworkBehaviour::inject_event
->::inject_handler_msg
With #2867
NetworkBehaviour::inject_event
would move toNetworkBehaviour::on_event
viaInEvent::ConnectionHandler
. (WhereInEvent
would be renamed toFromSwarm
with this proposal I am guessing.) Would you be in favor of that @elenaf9?
Yes, I think that makes sense. The only weird part is that FromSwarm
will have a ConnectionHandler
variant that will use the FromHandler
event. Perhaps #2867 should introduce two event handlers?
-
NetworkBehaviour::on_swarm_event(FromSwarm)
-
NetworkBehaviour::on_handler_event(FromHandler)
and
ConnectionHandler::inject_event
->inject_behaviour_msg
(or something similar).I suggest we do the same change on
ConnectionHandler
, see also #2832 (comment).
I suggest we try and pack all of these into a single release.
Yes, I think that makes sense. The only weird part is that FromSwarm will have a ConnectionHandler variant that will use the FromHandler event. Perhaps https://github.com/libp2p/rust-libp2p/pull/2867 should introduce two event handlers?
NetworkBehaviour::on_swarm_event(FromSwarm)
NetworkBehaviour::on_handler_event(FromHandler)
I'd be in favor of this.
Related to this: What do you think about renaming
NetworkBehaviour::inject_event
->::inject_handler_msg
With #2867
NetworkBehaviour::inject_event
would move toNetworkBehaviour::on_event
viaInEvent::ConnectionHandler
. (WhereInEvent
would be renamed toFromSwarm
with this proposal I am guessing.) Would you be in favor of that @elenaf9?Yes, I think that makes sense. The only weird part is that
FromSwarm
will have aConnectionHandler
variant that will use theFromHandler
event. Perhaps #2867 should introduce two event handlers?
NetworkBehaviour::on_swarm_event(FromSwarm)
NetworkBehaviour::on_handler_event(FromHandler)
and
ConnectionHandler::inject_event
->inject_behaviour_msg
(or something similar).I suggest we do the same change on
ConnectionHandler
, see also #2832 (comment).I suggest we try and pack all of these into a single release.
Being the rename of the associated types a breaking change by itself, should it be addressed when we soft (#3011/#3085) or hard deprecate the inject_*
methods? If we pack it with #3011/#3085 then it's probably redundant to soft deprecate the inject_*
methods since there will be a breaking change that implies touching the traits
Good point @jxs. Doing the rename with the hard deprecation (i.e. removal) of the inject_
methods sounds good to me.
Sounds reasonable to me. +1
I'm confused about why we keep saying Swarm
, I thought the term was replaced by Switch
Planning this for the next milestone.