Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/behaviour: Deprecate NetworkBehaviourEventProcess #2784
swarm/src/behaviour: Deprecate NetworkBehaviourEventProcess #2784
Changes from 4 commits
1a2e122
8d5f6a5
58baf57
4fd0266
0662d37
5e29c34
df81914
8c15452
b5cd98d
0f3ef69
22a4528
9b291c5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can we add the changelog entry above so this doesn't come up in the diff?
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.
I ordered the entries by significance. I don't think users care much about the
libp2p-core
update. I do think users care about how to upgrade fromNetworkBehaviourEventProcess
. Does that reasoning make sense @thomaseizinger?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.
What is the usage case for
NetworkBehaviourEventProcess
onEither
(orToggle
below)?As far as I understood it, the purpose of
NetworkBehaviourEventProcess
is only to be used in combination with theNetworkBehaviour
derive macro.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.
I guess when you use
Either
orToggle
when building a nestedNetworkBehaviour
. See for example:rust-libp2p/swarm-derive/tests/test.rs
Lines 354 to 369 in a4110a2
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.
Here with
NetworkBehaviourEventProcess
:rust-libp2p/swarm-derive/tests/test.rs
Lines 328 to 352 in 2f2b7cb
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.
But you wouldn't call the
NetworkBehaviourEventProcess
implementation of an inner behaviour, no? Instead you implementNetworkBehaviourEventProcess
on the parent (in the above case inFoo
) and handle the event there. Why would anyone want to inject the event back into an inner behaviour?None of our behaviours currently implement
NetworkBehaviourProcess
, so the below trait bound for it would not apply anyway:rust-libp2p/swarm/src/behaviour/toggle.rs
Lines 237 to 239 in a4110a2
So
libp2p::ping::Ping
does not implementNetworkBehaviourEventProcess
, thereforeToggle<libp2p::ping::Ping>
will also not implement it.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.
Unless I am missing something, there was never really a use-case for
NetworkBehaviourEventProcess
on those two behaviours in the first place. However, that's not really a concern of this PR.So I guess I am fine with for now with keeping it the way it is, since this trait will be remove anyway in the next release.