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

feat(rendezvous): directly return error from register #4073

Merged
merged 8 commits into from
Jun 17, 2023

Conversation

dgarus
Copy link
Contributor

@dgarus dgarus commented Jun 15, 2023

Description

Resolves #4070.

Notes & 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

@dgarus dgarus changed the title feat: rendezvous should synchronously return errors from behavior's register function feat: rendezvous should return result from register function Jun 15, 2023
@thomaseizinger thomaseizinger changed the title feat: rendezvous should return result from register function feat(rendezvous): directly return error from register function Jun 15, 2023
@thomaseizinger thomaseizinger changed the title feat(rendezvous): directly return error from register function feat(rendezvous): directly return error from register Jun 15, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks! Some minor comments and as you identified correctly, this is not backwards-compatible. That is fine because we are releasing breaking changes currently.

Can you add a changelog entry?

protocols/rendezvous/src/client.rs Show resolved Hide resolved
protocols/rendezvous/src/client.rs Outdated Show resolved Hide resolved
protocols/rendezvous/tests/rendezvous.rs Outdated Show resolved Hide resolved
examples/rendezvous/src/bin/rzv-identify.rs Outdated Show resolved Hide resolved
examples/rendezvous/src/bin/rzv-register.rs Outdated Show resolved Hide resolved
protocols/rendezvous/src/client.rs Show resolved Hide resolved
protocols/rendezvous/tests/rendezvous.rs Outdated Show resolved Hide resolved
@dgarus
Copy link
Contributor Author

dgarus commented Jun 15, 2023

Can you add a changelog entry?

Yes, of course.

@dgarus dgarus marked this pull request as ready for review June 16, 2023 07:59
@thomaseizinger
Copy link
Contributor

Can you fix the formatting?

@dgarus
Copy link
Contributor Author

dgarus commented Jun 16, 2023

Done

@dgarus
Copy link
Contributor Author

dgarus commented Jun 16, 2023

@thomaseizinger
Could you please recommend me an issue?
I'd like to study the next protocol, for example, kad.

@mergify mergify bot merged commit c4ab04c into libp2p:master Jun 17, 2023
@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jun 17, 2023

@thomaseizinger
Could you please recommend me an issue?
I'd like to study the next protocol, for example, kad.

Thank you!

Here are two issues that I'd consider a good fit if you want to dive into some of the protocols:

The first one just applies to the relay and contributes to the same overarching effort that you've just worked on.

The second one is a tracking issue with multiple tasks that also contributes to making the ConnectionHandler interface easier and hopefully allows protocols to compose better by taking away the ability to just close a connection which might interrupt another protocol's work.

@dgarus dgarus deleted the 4070 branch June 20, 2023 06:52
@dgarus
Copy link
Contributor Author

dgarus commented Jun 20, 2023

@thomaseizinger
Could you please recommend me an issue?
I'd like to study the next protocol, for example, kad.

Thank you!

Here are two issues that I'd consider a good fit if you want to dive into some of the protocols:

The first one just applies to the relay and contributes to the same overarching effort that you've just worked on.

The second one is a tracking issue with multiple tasks that also contributes to making the ConnectionHandler interface easier and hopefully allows protocols to compose better by taking away the ability to just close a connection which might interrupt another protocol's work.

Hi!
Both tasks are interesting. Think, I could start with 4075
Could you please assign it to me?

@thomaseizinger
Copy link
Contributor

Could you please assign it to me?

I think you will have to interact with the issue first before Github let's me do that :)

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

Successfully merging this pull request may close these issues.

rendezvous: synchronously return errors from behaviour functions
2 participants