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

added support for duration types #76

Merged
merged 2 commits into from
May 15, 2024
Merged

added support for duration types #76

merged 2 commits into from
May 15, 2024

Conversation

cocoa-xu
Copy link
Member

No description provided.

@josevalim
Copy link
Member

Elixir v1.17 will have a duration type. Do you think it should affect the design here in any way?

@cocoa-xu
Copy link
Member Author

Elixir v1.17 will have a duration type. Do you think it should affect the design here in any way?

This actually looks perfect for this function. Though it appears that the duration type won't support nanoseconds... (it should be ok as long as we convert nanoseconds to microseconds and make a note in the docs, but will you be interested in having nanosecond support for Duration...?)

@josevalim
Copy link
Member

Hrm, it is best to leave it separate as well then. Because Elixir calendar types do not support nanosecond. I think this is good to go as is then!

@cocoa-xu cocoa-xu merged commit e8545ae into main May 15, 2024
3 checks passed
@josevalim josevalim deleted the cx-duration branch May 15, 2024 12:21
@cocoa-xu
Copy link
Member Author

Hrm, it is best to leave it separate as well then.

No problem!

Because Elixir calendar types do not support nanosecond. I think this is good to go as is then!

Perhaps we can add nanoseconds for Calendar types as well? We can discuss this later ;)

@josevalim
Copy link
Member

It is hard to do so keeping backwards compatibility. Maybe we should support both duration and integers and drop precision for duration, but given we support integers already, this is good enough. In the future we can have options to keep the calendar types as integers instead of upcasting when reading from C.

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