-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Initial support for DateTime types #287
Conversation
This is just awesome — thank you very much for looking into this!
Exactly what I envisioned 👍
👍
I think this could be a second step maybe (and potentially needs some thought), to keep this PR manageable?
Ah, right. That is a bit unfortunate, yes. So far, we managed to decouple the unit- and dimension system (the prelude) fairly well from the language implementation. Maybe we can still achieve this if we 'register' the I also have a somewhat related question: are the current quantities of dimension A similar problem appears when we add/subtract I'm bringing this up here because we might need another special type anyway (maybe A much more conservative approach would be to only have functions like What do you think?
Despite what I said above, I think what you have already is really powerful! Calculations like The other thing I do frequently is to convert from/to UNIX timestamps, but that should be fairly easy to support with FFI functions. And then there are timezones. I would certainly like to see datetimes in the local timezone by default. But it would be great if we could also convert to other timezones somehow.
👍 |
Thanks for the initial review!
Agreed.
I do think something like this is needed, eventually (but maybe for a follow-up PR). Currently, if someone is building their own prelude that has a time-like dimension that isn't called "Time" (or has a different base-unit than Second), DateTimes won't work properly. But I'd like this to be possible.
Great question. In your BTW, there is an argument here that the current prelude definitions for I will say, however, that the current behavior of
I think these two examples suggest that we also need
For this, I'm wondering if we can re-purpose the conversion operator? For example, something like |
The same concern also applies to days: either it's always equal to 24 hours or you take DST and leap seconds into account. |
I added a few more commits that add some new datetime functions:
I also added experimental timezone support using the
|
I've been continuing to test drive this over the past week, and continue to find it very useful.
At this point, I think this PR is ready for another review and wider testing. |
Thank you very much for the updates. I currently don't have the energy to review this PR, but will hopefully do so soon. Concerning the question with calendar time units vs 'averaged' time units, I think it would be cool if someone could do some investigation how other tools handle this (e.g. Qalculate, Frink, GNU units, …) |
Thank you for the update. Please do not feel any rush or pressure to review this. I've opened a new discussion thread in #292 to track the research task into how other tools handle time units |
chrono = "0.4.31" | ||
chrono-tz = "0.8.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this has a quite drastic effect on the WASM size.
Before: 872.45 KB (872,448 bytes)
After: 1.89 MB (1,892,352 bytes)
I'm not saying this is a no-go, but maybe we could look into reducing the size of those dependencies (via feature flags?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I wasn't able to reduce the WASM size, even when disabled default features. chrono-tz has a way to limit the size of the TZ table which we might consider, but I'm not quite sure what ones we might omit (and I'm not currently sure how much space that would save anyway, compared to the size of chrono). It also recommends using LTO, but we're already using that in our release profile.
Suggestions from code review Co-authored-by: David Peter <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This looks cool, just a few minor nitpicks.
This lets us convert a Duration into an exact number of seconds without any unwraps or overflows
Co-authored-by: David Peter <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much!
This is an initial draft at adding DateTime handling into numbat. This is incomplete, but I'm opening the PR now to start a discussion about the general approach.
Some review notes:
DateTimes didn't really feel like any of the existing types, so they are an entirely new thing (so the parser, tokenize, AST, etc all are updated accordingly)
At the moment, the only way to construct a DateTime object is with the new
now()
function, but I envision some new date parsing functions (like astrptime
) at a minimum, and maybe dedicated handling in the parser for well-formatted datesTwo operations on DateTimes have been implemented:
Time
dimension) to aDateTime
to produce anotherDateTime
DateTime
s to produce something of typeTime
.Because of this dependency on having a
Time
dimension, the typechecker has some special code to check for times, which feels a bit ugly (but might be unavoidable)What other operations can we perform on dates?
Controlflow in typechecker is a bit of a mess as written, and deserves to be improved somehow
The
BinaryOperatorForDate
variant needs to be redesigned, I think.Tests and error handling is not added yet
I'm using
chrono
as the underlying date/time library.time
is another good choice. I pickedchrono
just because I have more personal experience with it. Both crates support wasm as far as I know.Small demo:
CC #181