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.

Sign up for GitHub

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

Jump to bottom

feat: add Sleeper based on futures-timer #154

Merged
merged 3 commits into from
Dec 27, 2024
Merged

feat: add Sleeper based on futures-timer #154

merged 3 commits into from
Dec 27, 2024

Conversation

NumberFour8
Copy link
Contributor

@NumberFour8 NumberFour8 commented Oct 17, 2024 •
edited
Loading

What

This PR adds the new FuturesTimerSleep implementation of a Sleeper which is based on the https://docs.rs/futures-timer/latest/futures_timer/ crate.

This timer is runtime agnostic and also works in WASM environments and is gated via futures-timer-sleep feature (enabled per default).

Closes #153

@NumberFour8 NumberFour8 mentioned this pull request Oct 17, 2024
Closed
Xuanwo
Xuanwo requested changes Oct 18, 2024
View reviewed changes
Copy link
Owner

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. But I prefer to keep gloo-timers feature as is and only add a new feature for futures-timer.

Xuanwo
Xuanwo reviewed Oct 18, 2024
View reviewed changes
backon/Cargo.toml Outdated Show resolved Hide resolved
backon/src/sleep.rs Outdated Show resolved Hide resolved
@NumberFour8
Copy link
Contributor Author

Thanks for the comments. I will improve it.

@NumberFour8
Copy link
Contributor Author

@Xuanwo I pushed some changes.

Let me know if a type alias for GlooTimersSleep is acceptable, or whether you'd like to have the whole previous implementation of the struct restored.

@NumberFour8 NumberFour8 requested a review from Xuanwo October 30, 2024 14:39
@Xuanwo
Copy link
Owner

Xuanwo commented Oct 30, 2024

Let me know if a type alias for GlooTimersSleep is acceptable, or whether you'd like to have the whole previous implementation of the struct restored.

Thank you for your efforts; I prefer to leave them unchanged.

@NumberFour8 NumberFour8 force-pushed the feature/add-futures-timer branch from 80e4f72 to d350da8 Compare December 25, 2024 19:58
@NumberFour8 NumberFour8 changed the title Replace gloo-timers with futures-timer feat: add Sleeper based on futures-timer Dec 25, 2024
@NumberFour8
Copy link
Contributor Author

Let me know if a type alias for GlooTimersSleep is acceptable, or whether you'd like to have the whole previous implementation of the struct restored.

Thank you for your efforts; I prefer to leave them unchanged.

Added the requested changes, pls check it out.

Xuanwo
Xuanwo reviewed Dec 26, 2024
View reviewed changes
backon/Cargo.toml Outdated
@@ -20,11 +20,12 @@ targets = [
]

[features]
default = ["std", "std-blocking-sleep", "tokio-sleep", "gloo-timers-sleep"]
default = ["std", "std-blocking-sleep", "tokio-sleep", "gloo-timers-sleep", "futures-timer-sleep"]
Copy link
Owner

Choose a reason for hiding this comment

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

Hi, please do not enable this sleep by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done

backon/src/sleep.rs Outdated
#[cfg(all(
not(feature = "tokio-sleep"),
not(feature = "gloo-timers-sleep"),
not(feature = "futures-timer-sleep")
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need to change this. futures-timer-sleep shoule not be part of dafault timer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@NumberFour8 NumberFour8 requested a review from Xuanwo December 26, 2024 18:12
Xuanwo
Xuanwo approved these changes Dec 27, 2024
View reviewed changes
Copy link
Owner

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @NumberFour8 for this!

@Xuanwo Xuanwo merged commit 45e9170 into Xuanwo:main Dec 27, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Why not using futures-timer ?
2 participants
@NumberFour8 @Xuanwo