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

Add convenience functions to transform between edgedb.Duration and time.Duration #304

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

lsdch
Copy link
Contributor

@lsdch lsdch commented Mar 18, 2024

This is meant to improve usability of Duration values and better integrate with the Go ecosystem.
This also makes the unit (micro/nanoseconds) difference between Duration and time.Duration more obvious, hopefully reducing the likelihood of introducing hard to detect bugs such as:

func ConvertDuration(d edgedb.Duration) time.Duration {
  return time.Duration(int64(d)) 
  // change from microseconds to nanoseconds representation is not taken care of, 
  // and the compiler can not help to prevent this
}

Copy link
Member

@fmoor fmoor left a comment

Choose a reason for hiding this comment

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

I like it!

internal/edgedbtypes/datetime.go Outdated Show resolved Hide resolved
Copy link
Member

@fmoor fmoor left a comment

Choose a reason for hiding this comment

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

Thanks for getting to this so quickly! Its been a while so I'm remembering a few more details as I look at this again.

internal/edgedbtypes/datetime_test.go Outdated Show resolved Hide resolved
internal/edgedbtypes/datetime.go Outdated Show resolved Hide resolved
internal/edgedbtypes/datetime_test.go Outdated Show resolved Hide resolved
Copy link
Member

@fmoor fmoor left a comment

Choose a reason for hiding this comment

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

Looks good. One last detail I forgot about.

internal/edgedbtypes/datetime.go Outdated Show resolved Hide resolved
@fmoor
Copy link
Member

fmoor commented Mar 20, 2024

Any last changes before I merge?

@lsdch
Copy link
Contributor Author

lsdch commented Mar 21, 2024

Nothing comes to mind, looks like it is ready 🚀

@fmoor fmoor merged commit d4da926 into edgedb:master Mar 22, 2024
9 checks passed
@fmoor
Copy link
Member

fmoor commented Mar 22, 2024

released in v0.17.0

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.

None yet

2 participants