Skip to content
This repository has been archived by the owner on Nov 23, 2019. It is now read-only.

Use time.Duration as type for timeouts #235

Merged
merged 2 commits into from
Jun 7, 2016
Merged

Use time.Duration as type for timeouts #235

merged 2 commits into from
Jun 7, 2016

Conversation

pdalpra
Copy link
Contributor

@pdalpra pdalpra commented May 25, 2016

This PR replaces instances of where timeout was an int representing the number of seconds by time.Duration, as suggested by #230 .
Not entirely sure about the float64 -> int conversion, also thought about using strconv.FormatFloat(v, 'f', 0, 64) for formatting it as an integer. WDYT ?

@vdemeester
Copy link
Contributor

@pdalpra cool 👍 Using strconv.FormatFloat makes more sence to me (reading it at least).

@pdalpra
Copy link
Contributor Author

pdalpra commented May 25, 2016

should I introduce this as a utility function or inline the code for the time being ?

@vdemeester
Copy link
Contributor

@pdalpra maybe a utility function it types/time 😝

@pdalpra
Copy link
Contributor Author

pdalpra commented Jun 7, 2016

sorry for keeping it stale, will update the PR today

@pdalpra
Copy link
Contributor Author

pdalpra commented Jun 7, 2016

@vdemeester PR (finally !) updated

@vdemeester
Copy link
Contributor

LGTM 👼
/cc @aaronlehmann @calavera

@pdalpra
Copy link
Contributor Author

pdalpra commented Jun 7, 2016

I can take of the matching PR on docker/docker if you like

@calavera
Copy link
Contributor

calavera commented Jun 7, 2016

LGTM

@calavera calavera merged commit 8c2141e into docker:master Jun 7, 2016
@pdalpra pdalpra deleted the timeout-as-time.Duration branch June 7, 2016 15:19
@MHBauer
Copy link
Contributor

MHBauer commented Jun 7, 2016

I'm a bit late to the review.

First, great thought and contribution @pdalpra. It is much appreciated and I think it is good practice to use time.Durations for our timeout durations.

I do not like that we convert an int to a float then parse the float to convert to a string.

  1. Too much conversion when it should be just number to number. (This is more of an aesthetics thing)
  2. This adopts float semantics for an integer. (I think this is mainly to be used with full seconds, so it does not matter so much)

My suggestion would to be add a comment on the rounding, just so no-one expects to get sub second precision, or to adopt what is in the godocs for time.Duration, i.e. the calculation becomes int64(duration/time.Second) instead of the float/string parsing. That also does a rounding (always round down) with no sub second precision.

Thanks again @pdalpra for the contribution here and in moby/moby#23344!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants