Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

swarm/src/lib: Remove Deref and DerefMut impls on Swarm #1995

Merged
merged 3 commits into from
Mar 18, 2021

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Mar 13, 2021

Remove Deref and DerefMut implementations previously dereferencing
to the NetworkBehaviour on Swarm. Instead one can access the
NetworkBehaviour via Swarm::behaviour and Swarm::behaviour_mut.
Methods on Swarm can now be accessed directly, e.g. via
my_swarm.local_peer_id().

Reasoning: Accessing the NetworkBehaviour of a Swarm through Deref
and DerefMut instead of a method call is an unnecessary complication,
especially for newcomers. In addition, Swarm is not a smart-pointer
and should thus not make use of Deref and DerefMut, see documentation
from the standard library below.

Deref should only be implemented for smart pointers to avoid
confusion.

https://doc.rust-lang.org/std/ops/trait.Deref.html


For the reasons mentioned above I would consider this breaking change worth doing. This has been shortly discussed in #1073 in the past. What do people think?

Remove `Deref` and `DerefMut` implementations previously dereferencing
to the `NetworkBehaviour` on `Swarm`. Instead one can access the
`NetworkBehaviour` via `Swarm::behaviour` and `Swarm::behaviour_mut`.
Methods on `Swarm` can now be accessed directly, e.g. via
`my_swarm.local_peer_id()`.

Reasoning: Accessing the `NetworkBehaviour` of a `Swarm` through `Deref`
and `DerefMut` instead of a method call is an unnecessary complication,
especially for newcomers. In addition, `Swarm` is not a smart-pointer
and should thus not make use of `Deref` and `DerefMut`, see documentation
from the standard library below.

> Deref should only be implemented for smart pointers to avoid
confusion.

https://doc.rust-lang.org/std/ops/trait.Deref.html
@thomaseizinger
Copy link
Contributor

thomaseizinger commented Mar 14, 2021

FWIW, I am in favor of this change. I always found Swarm::dial_addr hard to use because I know that it was there and it always takes me a while to remember that I have to call it as an associated function.

Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections from my side.

@mxinden
Copy link
Member Author

mxinden commented Mar 18, 2021

I would deem 5 days enough as a review period, thus merging now. Thanks for the reviews. Happy for future comments / suggestions.

@mxinden mxinden merged commit 63512e5 into libp2p:master Mar 18, 2021
@dvc94ch
Copy link
Contributor

dvc94ch commented Mar 20, 2021

Sorry I didn't see this sooner and comment. But I wonder if #2006 isn't the better approach. Keep the deref/deref_mut and instead removing the swarm methods/events in favor of more inject_*event* on NetworkBehaviour and more NetworkBehaviourActions. One wart of libp2p is having multiple ways of doing things. Users can register some channels or something if they really need to handle events outside of the swarm.

@thomaseizinger
Copy link
Contributor

@dvc94ch In my understanding, that is a orthogonal issue.

With this patch, we simply force users to explicitely type swarm.behaviour.foobar instead of just swarm.foobar. That is slightly more verbose but also more idiomatic Rust because Deref is only meant for smart-pointers.

As a result, we were able to remove the unintuitve associated functions for calling others things on Swarm like local_peer_id etc

Improving the NetworkBehaviour and all the things we talked about in the other thread are still possible as far as I can see :)

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 8, 2021

I think there are other cases where using Deref is valid, for example when creating a new type wrapper to implement a foreign trait on a foreign type.

But yes, I think it's orthogonal, I was just worried about too many api changes for users.

@MOZGIII
Copy link
Contributor

MOZGIII commented Apr 27, 2021

What do people think?

Thx so much for doing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants