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

Question: Right way to turn a Duration into milliseconds? #10679

Closed
Araq opened this issue Feb 15, 2019 · 6 comments · Fixed by #10710
Closed

Question: Right way to turn a Duration into milliseconds? #10679

Araq opened this issue Feb 15, 2019 · 6 comments · Fixed by #10710
Assignees
Labels

Comments

@Araq
Copy link
Member

Araq commented Feb 15, 2019

This does what the documentation says it would do, I guess, but how to accomplish what I'm trying to do?

import times
var a = initDuration(seconds = 2)
echo a.milliseconds()

Outputs: 0
Expected: 2000

@andreaferretti
Copy link
Collaborator

I guess that's not well-defined?

import times
var a = initDuration(months = 2)
echo a.days()

@andreaferretti
Copy link
Collaborator

Uh sorry, a Duration cannot have months. Still, given leap seconds, anything above minutes could not be convertible exactly

@GULPF
Copy link
Member

GULPF commented Feb 15, 2019

@andreaferretti Duration defines a day as exactly 86400 seconds, so it's not an issue in this context.

The reason that I implemented dur.milliseconds this way was because I didn't want something that looks like a field access to cause overflows. I'm not happy about the behavior though, it's confusing and not very useful. To do conversion to milliseconds something like this is required: convert(Seconds, Milliseconds, a.seconds) + a.milliseconds .

@GULPF
Copy link
Member

GULPF commented Feb 15, 2019

To be honest, maybe the conversion shorthands should just be deprecated and we can add a proc convert(Duration, FixedTimeUnit): int64 instead. That makes it clearer that an overflow can occur, and reduces the api surface. There's way to many one line shorthand procs in the times module.

@dom96
Copy link
Contributor

dom96 commented Feb 16, 2019

To be honest, maybe the conversion shorthands should just be deprecated and we can add a proc convert(Duration, FixedTimeUnit): int64 instead.

I think a better option would be to create a asMilliseconds function. It makes it clearer that a conversion is occurring. Only problem I see with this is that it might not be consistent with how we do conversions in the rest of the stdlib, but I don't actually have that on top of my head so it can't be too consistent as it is right now...

@Araq
Copy link
Member Author

Araq commented Feb 17, 2019

Yeah give us asMilliseconds or inMilliseconds shortcuts, convert is too confusing. :-)

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

Successfully merging a pull request may close this issue.

4 participants