Skip to content

feat: Accept durations as timeout arguments#172

Merged
hoodie merged 2 commits intohoodie:mainfrom
nicos68:duration
Jan 7, 2023
Merged

feat: Accept durations as timeout arguments#172
hoodie merged 2 commits intohoodie:mainfrom
nicos68:duration

Conversation

@nicos68
Copy link
Copy Markdown
Contributor

@nicos68 nicos68 commented Dec 18, 2022

Comment thread src/timeout.rs Outdated
@nicos68 nicos68 changed the title Accept durations as timeout arguments feat: Accept durations as timeout arguments Dec 27, 2022
Comment thread src/timeout.rs
impl From<Duration> for Timeout {
fn from(duration: Duration) -> Timeout {
if duration.is_zero() {
Timeout::Never
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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