search

docker / go-units

Parse and print size and time units in human-readable format
https://godoc.org/github.com/docker/go-units
Apache License 2.0
218 stars 38 forks source link

return uint64 for parseSize #19

Closed allencloud closed 8 years ago

allencloud commented 8 years ago

As parseSize already returns error, so I think there is no need to return -1. Then we can change returned type from int64 to uint64.

Signed-off-by: allencloud allen.sun@daocloud.io

allencloud commented 8 years ago

Any update?

dnephin commented 8 years ago

Seems ok to me, but this change is far from backwards compatible.

Is it worth breaking everyone who is using this library?

allencloud commented 8 years ago

What you said is reasonable. @dnephin Thanks for your feedback.

Actually, I did this PR for two reasons:

  1. -1 is somewhat redundant in this case, as we already have an error, right?
  2. Here is some kind of demand in docker engine : https://github.com/docker/docker/blob/master/runconfig/opts/parse.go#L325

While, to be honest, backwards compatibility is something I missed, sorry for that. :)

thaJeztah commented 8 years ago

Should we close this PR?

dnephin commented 8 years ago

If we can't make it backwards compatible I think we should close it. I understand it is more correct, but I'm not sure we gain enough from it to warrant the breaking change.

allencloud commented 8 years ago

Yes. What you said is totally reasonable. Thanks a lot for your review. @dnephin @thaJeztah I am closing it.

thaJeztah commented 8 years ago

Thanks @allencloud