-
Notifications
You must be signed in to change notification settings - Fork 998
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
Make it easier to define implementations of NetworkBehaviour
#2150
Conversation
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.
dc690e4
to
f90c46b
Compare
There was a problem hiding this 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?
swarm/src/behaviour.rs
Outdated
_: PeerId, | ||
_: ConnectionId, | ||
_: <<Self::ProtocolsHandler as IntoProtocolsHandler>::Handler as ProtocolsHandler>::OutEvent | ||
) { } |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this 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
Ah damn. Still need to get used to my new RA based setup ... |
Can you take another look at the current failure @thomaseizinger?
|
Argh, I ran clippy without |
Hopefully everything is done now @mxinden ! Sorry about the mess, transitioning local setup at the moment and not quite used to it yet ... |
Thanks @thomaseizinger! |
See patches for details.
Not sure how controversial this proposal is :)