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

feat: Accept durations as timeout arguments #172

Merged
merged 2 commits into from
Jan 7, 2023
Merged

Conversation

nicos68
Copy link
Contributor

@nicos68 nicos68 commented Dec 18, 2022

src/timeout.rs Outdated Show resolved Hide resolved
@nicos68 nicos68 changed the title Accept durations as timeout arguments feat: Accept durations as timeout arguments Dec 27, 2022
impl From<Duration> for Timeout {
fn from(duration: Duration) -> Timeout {
if duration.is_zero() {
Timeout::Never
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it's a good idea to convert Duration 0 to Never just because the 0 value means never.
If you handover a parameter of type Duration then this behavior would be very unexpected.
What if we convert a timeout of Duration of 0ms into a None timeout.

Copy link
Contributor Author

@nicos68 nicos68 Jan 7, 2023

Choose a reason for hiding this comment

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

But we have to return a Timeout object, we can't return Option<Timeout> (or at least I don't see how). Then either we take the user at their word and return Milliseconds(0) or we try to be smarter then them and return Default. What do you think ?

Copy link
Owner

Choose a reason for hiding this comment

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

oh yes, you're right

Copy link
Owner

Choose a reason for hiding this comment

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

I guess it's just weird API. then let's leave it like this. Thanks for the change, I'll just add a bit more documentation to make sure this is explained to the user.

@hoodie hoodie merged commit 940a54b into hoodie:main Jan 7, 2023
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.

2 participants