-
Notifications
You must be signed in to change notification settings - Fork 25
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 IfWatcher::new synchronous #24
Conversation
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.
👍 Cool. This looks good to me.
@elenaf9 could you resolve the merge conflicts? |
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.
Oh, missing changelog entry.
@@ -8,8 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|||
### Changed | |||
- Add `IfWatcher::poll_next`. Implement `Stream` instead of `Future` for `IfWatcher`. See [PR 23]. | |||
- Make `IfWatcher::new` synchronous. See [PR 24]. |
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.
Not sure how to describe this better.
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.
@@ -8,8 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|||
### Changed | |||
- Add `IfWatcher::poll_next`. Implement `Stream` instead of `Future` for `IfWatcher`. See [PR 23]. | |||
- Make `IfWatcher::new` synchronous. See [PR 24]. |
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.
@elenaf9 should I cut a release already, or do you expect other changes in the near future? |
I am still looking into simplifying the |
Make
IfWatcher::new
synchronous. The only reason why it is currently async is because thelinux
implementation executed aAddressGetRequest
innew
to obtain the initial list of ip addresses. By chaining this stream together with the existing message stream we can makelinux::IfWatcher::new
sync and instead move the polling into theStream
impl.Motivation for this change is that
IfWatcher::new
being async currently causes some complexity in rust-libp2p. Concretely, it forces to wrap theIfWatcher
in an enum [1] and first poll the future returned byIfWatcher::new
[2] before we can poll theIfWatcher
itself.