-
Notifications
You must be signed in to change notification settings - Fork 27
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
*: Replace Future impl with poll_next method #23
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove Future impl on all platform IfWatcher's, instead add `poll_next` method. Implement `Stream` and `FusedStream` for user-facing IfWatcher.
mxinden
reviewed
Aug 9, 2022
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 apart from the version bump.
elenaf9
commented
Aug 9, 2022
mxinden
approved these changes
Aug 10, 2022
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.
👍
@elenaf9 can you take a look at the CI failure?
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #22.
Remove
Future
implementation on allIfWatcher
s, instead addpoll_next
method to poll for address changed.For the user-facing
IfWatcher
addStream
andFusedStream
for convenience.Probably easier to review the two commits separately.
466bd81 moves the poll logic from the
Future
impl into the newpoll_next
method and addsStream
+FusedStream
.9082c0e slightly refactors some of the
poll_next
implementation so that they are somewhat unified (following the conventions described in libp2p/rust-libp2p#2780):I think this makes it a bit easier to understand the different implementations, but I am fine with dropping that second commit in case that there are any concerns about the changes.