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

*: Replace Future impl with poll_next method #23

Merged
merged 5 commits into from Aug 10, 2022

Conversation

elenaf9
Copy link
Contributor

@elenaf9 elenaf9 commented Aug 5, 2022

Fixes #22.

Remove Future implementation on all IfWatchers, instead add poll_next method to poll for address changed.
For the user-facing IfWatcher add Stream and FusedStream for convenience.

Probably easier to review the two commits separately.
466bd81 moves the poll logic from the Future impl into the new poll_next method and adds Stream + 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):

  • wrap the polling logic in a loop
  • first return existing queued events
  • poll for new events, continue in loop if progress was made

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.

Remove Future impl on all platform IfWatcher's, instead add `poll_next`
method. Implement `Stream` and `FusedStream` for user-facing IfWatcher.
Copy link
Owner

@mxinden mxinden left a 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.

Cargo.toml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Owner

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

👍

@mxinden mxinden changed the title *: replace Future impl with poll_next method *: Replace Future impl with poll_next method Aug 10, 2022
@mxinden
Copy link
Owner

mxinden commented Aug 10, 2022

@elenaf9 can you take a look at the CI failure?

error: unused import: `std::future::Future`
Error:   --> src/apple.rs:10:5
   |
10 | use std::future::Future;
   |     ^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> src/lib.rs:3:9
   |
3  | #![deny(warnings)]
   |         ^^^^^^^^
   = note: `#[deny(unused_imports)]` implied by `#[deny(warnings)]`

error: could not compile `if-watch` due to previous error

@mxinden mxinden merged commit 8ddf200 into mxinden:master Aug 10, 2022
@elenaf9 elenaf9 deleted the issue-22 branch August 10, 2022 08:56
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.

Implement Stream instead of Future for IfWatcher
2 participants