-
Notifications
You must be signed in to change notification settings - Fork 949
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(swarm): report outcome of handling SwarmEvent
#3865
Conversation
This pull request has merge conflicts. Could you please resolve them @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.
Looks good to me. Though I would prefer not using once_cell::Lazy
here for the sake of simplicity.
static MEMORY_ADDR: Lazy<Multiaddr> = | ||
Lazy::new(|| Multiaddr::empty().with(Protocol::Memory(1000))); |
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 value of a static
over a const
here? Why does it warrant introducing once_cell
as a dependency?
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.
Multiaddr::empty
and Multiaddr::with
are unfortunately not const
.
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.
Ah, right. I missed that.
I am hesitant to introduce a dependency just for the sake of more succinct test code. That said, once_cell
seems to be everywhere, thus not really an additional dependency.
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.
It is on the pathway to being stabilized and part of std: rust-lang/rust#105587
static MEMORY_ADDR: Lazy<Multiaddr> = | ||
Lazy::new(|| Multiaddr::empty().with(Protocol::Memory(1000))); |
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.
Ah, right. I missed that.
I am hesitant to introduce a dependency just for the sake of more succinct test code. That said, once_cell
seems to be everywhere, thus not really an additional dependency.
Description
Previously, a user wouldn't know whether passing a
SwarmEvent
toListenAddresses
orExternalAddresses
changed the state. We now return a boolean wheretrue
indicates that we handled the event and changed state as a result.The API is inspired by
HashSet::insert
and the like.Notes & open questions
Extracted out of #3651.
Change checklist