rust-libp2p icon indicating copy to clipboard operation
rust-libp2p copied to clipboard

swarm/handler: Rename `InEvent` and `OutEvent` to `FromBehaviour` and `ToBehaviour`

Open thomaseizinger opened this issue 2 years ago • 9 comments

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.

thomaseizinger avatar Aug 30 '22 09:08 thomaseizinger

I find this more intuitive looking at ConnectionHandler only. Though in the greater picture, including NetworkBehaviour, it is inconsistent with NetworkBehaviour::OutEvent.

mxinden avatar Sep 02 '22 03:09 mxinden

I find this more intuitive looking at ConnectionHandler only. Though in the greater picture, including NetworkBehaviour, it is inconsistent with NetworkBehaviour::OutEvent.

We can name that one ToSwarm? :)

thomaseizinger avatar Sep 02 '22 06:09 thomaseizinger

+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.

elenaf9 avatar Sep 02 '22 14:09 elenaf9

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.

elenaf9 avatar Sep 02 '22 15:09 elenaf9

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.

Same! Especially the ConnectionHandlers 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.

thomaseizinger avatar Sep 04 '22 09:09 thomaseizinger

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.

mxinden avatar Sep 05 '22 06:09 mxinden

I am in favor of the proposals in this issue.

mxinden avatar Sep 05 '22 06:09 mxinden

Related to this: What do you think about renaming NetworkBehaviour::inject_event -> ::inject_handler_msg

With #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?

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.

thomaseizinger avatar Sep 05 '22 17:09 thomaseizinger

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.

elenaf9 avatar Sep 07 '22 08:09 elenaf9

Related to this: What do you think about renaming NetworkBehaviour::inject_event -> ::inject_handler_msg

With #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?

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.

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

jxs avatar Nov 04 '22 14:11 jxs

Good point @jxs. Doing the rename with the hard deprecation (i.e. removal) of the inject_ methods sounds good to me.

mxinden avatar Nov 04 '22 20:11 mxinden

Sounds reasonable to me. +1

thomaseizinger avatar Nov 04 '22 21:11 thomaseizinger

I'm confused about why we keep saying Swarm, I thought the term was replaced by Switch

Mubelotix avatar Nov 07 '22 17:11 Mubelotix

Planning this for the next milestone.

thomaseizinger avatar Mar 22 '23 14:03 thomaseizinger