Skip to content

Conversation

@v0idpwn
Copy link
Collaborator

@v0idpwn v0idpwn commented Apr 25, 2025

Can't use duration.second because you may have minutes, weeks, etc to consider.

@v0idpwn v0idpwn requested a review from whatyouhide April 25, 2025 22:23
@spec from_duration(Duration.t()) :: Google.Protobuf.Duration.t()
def from_duration(%Duration{} = duration) do
{microsecond, _precision} = duration.microsecond
seconds = div(to_timeout(duration), 1000)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I expected elixir to have a better API for that, but I didn't find one 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is the correct implementation. duration.microsecond is of type Calendar.microsecond/0, and the second element of the tuple should be used in the calculation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

From a couple weeks ago: elixir-lang/elixir@dd608c9

Copy link
Collaborator Author

@v0idpwn v0idpwn Apr 28, 2025

Choose a reason for hiding this comment

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

TIL :)
I thought it was only info about how many digits of precision we could expect.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL ... I needed that extra context around precision as well, I was YOLO it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, @whatyouhide, per our discussion, this is actually correct, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I expected elixir to have a better API for that, but I didn't find one 🤔

@v0idpwn you can use System.convert_time_unit/3.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't mean the div/2 but the to_timeout. I would like to have Duration.to_unit(duration, :second)

@whatyouhide whatyouhide merged commit 451133b into main Apr 30, 2025
4 checks passed
@whatyouhide whatyouhide deleted the fix/duration-conversion branch April 30, 2025 15:47
@yordis
Copy link
Contributor

yordis commented Apr 30, 2025

@v0idpwn @whatyouhide do you think you could cut a release with the feature?

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.

4 participants