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

Make it easier to define implementations of NetworkBehaviour #2150

Merged
merged 15 commits into from
Jul 30, 2021

Conversation

thomaseizinger
Copy link
Contributor

See patches for details.

Not sure how controversial this proposal is :)

Not all implementations of `NetworkBehaviour` need all callbacks.
We've have been adding new callbacks with default implementations
for a while now. There is no reason the initial ones cannot also
be defaulted, thus making it easier create new implementations.
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Looks good to me other than inject_event. What do you think of my comment below @thomaseizinger?

_: PeerId,
_: ConnectionId,
_: <<Self::ProtocolsHandler as IntoProtocolsHandler>::Handler as ProtocolsHandler>::OutEvent
) { }
Copy link
Member

Choose a reason for hiding this comment

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

A NetworkBehaviour that does not receive any events from its ProtocolsHandler should use void::Void on their ProtocolsHandler::OutEvent, thus checked at compile time, no?

I would prefer the above over a default implementation. What do you think? If you agree, could you add a doc comment to inject_event?

Copy link
Contributor Author

@thomaseizinger thomaseizinger Jul 24, 2021

Choose a reason for hiding this comment

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

We can do that yeah.

I approached this more from a prototyping perspective: If I want to define a new NetworkBehaviour, what is the least amount of stuff I am faced with?

I was also tempted to default poll actually. An AddressBook style NetworkBehaviour doesn't need poll for example :)

I am not sure inject_event with match event { } is actually cleaner. The only thing it protects you from is forgetting to deal with your events if you change the ProtocolsHandler.

For this to work, it implies that people actually override it with match event { } in the first place and not just leave it blank. At least rust-analyzer autocompletes trait-impls with todo!() though and the path of least resistance is to either leave those in or delete them 🤷‍♂️

In any case, defaulting the others is already an improvement so I'll do that.

swarm/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

@thomaseizinger can you fix the failing tests below? Other than that, this looks good to me.

error[E0046]: not all trait items implemented, missing: `inject_event`
   --> swarm/src/test.rs:68:1
   ::: swarm/src/behaviour.rs:130:5

@thomaseizinger
Copy link
Contributor Author

@thomaseizinger can you fix the failing tests below? Other than that, this looks good to me.

error[E0046]: not all trait items implemented, missing: `inject_event`
   --> swarm/src/test.rs:68:1
   ::: swarm/src/behaviour.rs:130:5

Ah damn. Still need to get used to my new RA based setup ...

@mxinden
Copy link
Member

mxinden commented Jul 28, 2021

Can you take another look at the current failure @thomaseizinger?

error[E0223]: ambiguous associated type
  --> swarm/src/test.rs:89:12
   |
89 |         _: ProtocolsHandler::OutEvent
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use fully-qualified syntax: `<Type as protocols_handler::ProtocolsHandler>::OutEvent`

error: aborting due to previous error

@thomaseizinger
Copy link
Contributor Author

Can you take another look at the current failure @thomaseizinger?

error[E0223]: ambiguous associated type
  --> swarm/src/test.rs:89:12
   |
89 |         _: ProtocolsHandler::OutEvent
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use fully-qualified syntax: `<Type as protocols_handler::ProtocolsHandler>::OutEvent`

error: aborting due to previous error

Argh, I ran clippy without --all-targets.

@thomaseizinger
Copy link
Contributor Author

Hopefully everything is done now @mxinden !

Sorry about the mess, transitioning local setup at the moment and not quite used to it yet ...

@mxinden mxinden merged commit ad90167 into libp2p:master Jul 30, 2021
@mxinden
Copy link
Member

mxinden commented Jul 30, 2021

Thanks @thomaseizinger!

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.

2 participants