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-derive/: Generate OutEvent if not provided #2792

Merged
merged 5 commits into from
Aug 8, 2022

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Aug 2, 2022

Description

Generate NetworkBehaviour::OutEvent if not provided through
#[behaviour(out_event = "MyOutEvent")] and event processing is
disabled (default).

Extracted from #2751.

As suggested by @elenaf9 in #2784 (review) we would do the following:

  1. Merge this pull request.
  2. Merge the pull request deprecating NetworkBehaviourEventProcess.
  3. Cut a new release.
  4. Remove NetworkBehaviourEventProcess via swarm*/: Remove NetworkBehaviourEventProcess and generate OutEvent #2751.

What do folks think?

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Generate `NetworkBehaviour::OutEvent` if not provided through
`#[behaviour(out_event = "MyOutEvent")]` and event processing is
disabled (default).
@@ -79,16 +79,11 @@ pub(crate) type THandlerOutEvent<THandler> =
/// it will delegate to each `struct` member and return a concatenated array of all addresses
/// returned by the struct members.
///
/// When creating a custom [`NetworkBehaviour`], you must choose one of two methods to respond to
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that the documentation on NetworkBehaviour now no longer documents NetworkBehaviourEventProcess. My rational being that we will deprecate it anyways, thus there is no use in pointing newcomers to it.

Any objections?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope.

Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Maybe I overlooked it, but is there a test that validates for a generated OutEvent that it looks as expected? Could be done by implementing a simple trait for the generated OutEvent or something, e.g. impl TryInto<libp2p::kad::KademliaEvent> for FooEvent (for each inner behaviour).

@thomaseizinger
Copy link
Contributor

Maybe I overlooked it, but is there a test that validates for a generated OutEvent that it looks as expected? Could be done by implementing a simple trait for the generated OutEvent or something, e.g. impl TryInto<libp2p::kad::KademliaEvent> for FooEvent (for each inner behaviour).

We could also write a function that matches on an instance of the enum. Should also validate the enum structure and the variant names.

@mxinden
Copy link
Member Author

mxinden commented Aug 4, 2022

@elenaf9 @thomaseizinger good idea. What do you think of b4e32ed?

Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Lgtm!

Comment on lines 50 to 52
FooEvent::Ping(event) => {
let _: libp2p::ping::Event = event;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just extend the pattern match?

Suggested change
FooEvent::Ping(event) => {
let _: libp2p::ping::Event = event;
}
FooEvent::Ping(libp2p::ping::Event { .. }) => { }

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in ff63681. Though unfortunately I don't see a way to do the same for the identify and kademlia event given that they are enums.

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.

3 participants