-
Notifications
You must be signed in to change notification settings - Fork 52
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
[Feature] Implemented Datetime object support #675
Conversation
Looks like there is some issue in handling gmt in windows, will fix it |
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.
Couple of minor points
src/optionals/datetime.c
Outdated
// defineNative(vm, &module->values, "now", nowNative); | ||
// defineNative(vm, &module->values, "nowUTC", nowUTCNative); |
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.
If these arent needed we can remove
// defineNative(vm, &module->values, "now", nowNative); | |
// defineNative(vm, &module->values, "nowUTC", nowUTCNative); |
tests/datetime/constants.du
Outdated
@@ -15,30 +15,30 @@ class TestDatetimeConstants < UnitTest { | |||
const DATE_TIME_FORMAT = "%a %b %d %H:%M:%S %Y"; | |||
|
|||
testSecondsInMinuteConstant() { | |||
const startSec = Datetime.strptime(this.DATE_TIME_FORMAT, "Fri May 29 03:12:32 2020"); | |||
const startSec = Datetime.new(this.DATE_TIME_FORMAT, "Fri May 29 03:12:32 2020").strptime(); |
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.
For these can we leave the existing tests alone but also add in new tests?
Appreciate the work you've done on this so i've stuck the hacktoberfest label on it (if you're participating) until I get some time to thoroughly check it and get it merged in! |
I think I'm going to work on this PR (unless you want to continue with it) so that all of the object methods return a Datetime object rather than primitive values and have extra methods on the datetime object to convert to string / numeric values (toString like you have, or toEpoch). Means as we add more methods we can much easier chain things (e.g like addHour or whatever) |
Certainly, could you provide some contract ideas that I can attempt to implement ?. |
Exactly yeah, we'd leave the current ones in and they can return the primitive values. Your new object methods would instead return a Datetime object, so:
|
Thanks for Suggestions, Methods of Datetime moduleAlready thereDatetime.new(); -> Datetime(Object) , current time object To implement/modifyDatetime.new(); -> Datetime (Object) Methods of Datetime ObjectAlready theredateTime(object).strftime() -> String , returns string In futuredateTime(object).addMinutes(); -> Datetime(Object) |
Apologies yeah, strptime to return a date time object rather than a number, and then getTime to return a number I had <values> because at the moment this is overloaded, that’s fine by me and can stay as is |
No rush or anything @MannarAmuthan but just checking if you're still working on this? |
Hi yes , I am planning to work on this in weekend |
Hi @Jason2605 , Please look into it, and provide any suggestions or changes needed. |
Will take a look later today, thanks @MannarAmuthan |
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.
Thanks for this!! Few small comments really
src/optionals/datetime.c
Outdated
UNUSED(args); | ||
|
||
if (argCount != 0) { | ||
runtimeError(vm, "nowNative() takes no arguments (%d given)", argCount); |
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.
runtimeError(vm, "nowNative() takes no arguments (%d given)", argCount); | |
runtimeError(vm, "now() takes no arguments (%d given)", argCount); |
src/optionals/datetime.c
Outdated
UNUSED(args); | ||
|
||
if (argCount != 0) { | ||
runtimeError(vm, "nowUTCNative() takes no arguments (%d given)", argCount); |
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.
runtimeError(vm, "nowUTCNative() takes no arguments (%d given)", argCount); | |
runtimeError(vm, "nowUTC() takes no arguments (%d given)", argCount); |
src/optionals/datetime.c
Outdated
UNUSED(args); | ||
|
||
if (argCount != 0) { | ||
runtimeError(vm, "nowUTCNative() takes no arguments (%d given)", argCount); |
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.
runtimeError(vm, "nowUTCNative() takes no arguments (%d given)", argCount); | |
runtimeError(vm, "newUTC() takes no arguments (%d given)", argCount); |
src/optionals/datetime.c
Outdated
|
||
if(argCount == 1) { | ||
if (!IS_NUMBER(args[0])) { | ||
runtimeError(vm, "new() takes 1 argument , must be a number"); |
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.
runtimeError(vm, "new() takes 1 argument , must be a number"); | |
runtimeError(vm, "new() argument must be a number"); |
src/optionals/datetime.c
Outdated
return OBJ_VAL(newDatetimeObj(vm, tictoc.tm_sec, tictoc.tm_min, tictoc.tm_hour, tictoc.tm_mday, tictoc.tm_mon, tictoc.tm_year)); | ||
} | ||
|
||
runtimeError(vm, "new() takes 0 or 1 argument, (%d given)", argCount); |
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.
runtimeError(vm, "new() takes 0 or 1 argument, (%d given)", argCount); | |
runtimeError(vm, "new() takes 0 or 1 arguments (%d given)", argCount); |
docs/docs/standard-lib/datetime.md
Outdated
datetime.strftime("Today is %A"); // Today is Friday | ||
``` | ||
|
||
Returns in default format when user defined format is not provided |
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.
Be nice to note the default format here: %a %b %d %H:%M:%S %Y
src/optionals/datetime.c
Outdated
time_t t = time(NULL); | ||
struct tm tictoc; | ||
char time[26]; | ||
char *queueString = malloc(sizeof(char) * 11); |
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.
char *queueString = malloc(sizeof(char) * 11); | |
char *datetimeString = malloc(sizeof(char) * 11); |
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.
*And same below
src/optionals/datetime.c
Outdated
if (argCount != 1 && argCount != 2) { | ||
runtimeError(vm, "strftime() takes 1 or 2 arguments (%d given)", argCount); | ||
if (argCount > 1) { | ||
runtimeError(vm, "strftime() takes 0 or 1 argument (%d given)", argCount); |
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.
runtimeError(vm, "strftime() takes 0 or 1 argument (%d given)", argCount); | |
runtimeError(vm, "strftime() takes 0 or 1 arguments (%d given)", argCount); |
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.
These suggestions are done
@briandowns Be good to get your thoughts on this too |
I like this. I'm curious if we could simplify it though. I was looking at working on a time module myself at one point and was going to model it after the Time package in Go. Essentially, the Time type is just a number and operations are performed on that value(s). There's a Duration type as well which is just an alias for an int64. The package's API is small and clean as well. https://cs.opensource.google/go/go/+/master:src/time/time.go;drc=bc2124dab14fa292e18df2937037d782f7868635;l=135 |
Hi @briandowns , That one is great reference, Thanks for this ! I assume we are talking about the structure of datetime object , will take look into it. |
Yeah this is an interesting concept, will need to have a deeper look through the Go source but I think representing the time as a number internally makes a lot of sense to me |
Actually I read more on this, and found that the classic time_t epoch integer, represents time after 1970. I changed the code, it is working fine, New structure: typedef struct { But I worry, that we need the capability to represent/create Datetime before 1970 also. |
As always, I'll defer to @Jason2605 but I think we can be fine with storing a wall time and mono time in int64_t values in a dateTime object. From there, that single object can be used for timers, time diffs, and we can cast up and down from time_t. In the case of timestamps prior to epoch, they end up being negative numbers. |
Honestly for this one, I'd probably go with whatever the go library is doing (still need to properly look at it sorry). A single numeric value storing the current time sounds perfect to me and going into negative numbers for dates prior to 1970 is fine (and what happens when you want a time before then when using unix time anyways yeah) |
A simple and naive assumption of how this might look: typedef int64_t Duration;
Duration microseconds(const Duration d) {
return (Duration)d * 1e3;
}
Duration milliseconds(const Duration d) {
return (Duration)d * 1e6;
}
Duration parseDuration(const char *s) {
// convert strings like "1s" to a duration of 1 second
Duration d;
return d;
}
typedef struct {
int64_t wall;
int64_t mono;
} DateTime;
DateTime now() {
struct timespec wall, mono;
clock_gettime(CLOCK_MONOTONIC, &mono);
clock_gettime(CLOCK_REALTIME, &wall);
DateTime dt;
dt.mono = mono.tv_sec;
dt.wall = wall.tv_sec;
return dt;
}
DateTime add(const DateTime dt, Duration d) {
DateTime ndt;
ndt.wall += dt.wall;
return ndt;
}
DateTime sub(const DateTime dt, Duration d) {
DateTime ndt;
ndt.wall -= dt.wall;
return ndt;
}
Duration since(const DateTime dt, const Duration d) {
DateTime ndt = sub(now(), d);
return (Duration)ndt.wall;
} Rooted off of the DateTime class, we can any number of methods that will use the values therein. Timers using mono, time and date operations using wall. |
Thanks for all suggestions, I am planning to work on this , will provide updates |
Currently, I have created a datetime object to store time as an integer and included a flag for location (local or not). However, this is not quite complete; I need to implement ADD and SUB methods ( and duration ) to ensure proper functionality. I will investigate this further and continue working on it. |
Thanks for still working on this! Will be a really nice addition to get a more fully featured datetime library |
Suggestion and Approaches for introducing timedelta (or) duration object:Naming: The name timedelta is inspired from python datetime package , meanwhile duration also makes sense Note: These are just initial thoughts, welcom more suggestions, even hybrid approaches. To Remember: Approach 1: Constructing TimeDelta by dictionary
Pros
Cons
Approach 2: Avoid delta object
or
Pros
Cons
|
Thanks very much for the detailed write up! I personally think the Timedelta option is the way to go - I feel like when dealing with calculating time differences it will be a lot easier to conditionally create a timedelta object than it will be to apply different methods - it also means we will be able to have an actual value representation of a length of time (we can then apply nice string methods to this for example)! No kwargs is a valid gripe and would be a really nice addition, but as you say for now, a dictionary would be the next best replacement for instantiation Be good to hear your thoughts too @briandowns |
I like the idea of a timedelta object. I have one tiny thought though. I'm more of a fan of having time objects be immutable since they represent a single point on a linear track. If we need to adjust the object by adding or subtracting time, I'd like a new object. We then have the ability to do compare operations. |
Yeah when you run I'm not too fussed on Duration vs Timedelta either, Luxon a popular JS library seems to use Duration whereas Python's implementation as mentioned uses Timedelta, so open to either on the naming front. Comparisons of objects will be an interesting one in the sense of would you expect the standard operators to work with these types (< > != ==) or would we have comparison methods (potentially opens up the question of user defined operator overloading too but can save that for another day) |
I'm not too concerned around naming Go uses an int64 as type Duration. Meh. As for object comparison, I'd probably at least expect a couple methods like t2.equals(t1);, and maybe a diff method. |
So you'd prefer methods rather than overloading the operators? Makes sense, yeah I think im siding towards |
Yeah, for sure for methods over operators. Seems like the surface area for that is super small and still achieves the necessary functionality. |
If this has stagnated, I'd like to pick it up. |
Have at it mate! |
Sorry for dropping this earlier, but I'd love to jump back in and help now. Let me know if there are any updates or changes needed from my side, and I'll get on it as soon as possible. I guess, I've finished most of the things, but I am not sure about the failures. Need to check |
I out up a new PR. Have a look and let me know your thoughts. |
Closing in favour of #750 |
Implemented Datetime object
Resolves: #647
What's Changed:
I didn't removed old contracts yet, means there is no any breaking changes, confirmed with tests. Added instantiation for datetime objects, once we agreed on these contracts, other utility methods for this datetime objects can be implemented on top of it.
Type of Change:
Housekeeping:
Added methods for datetime object
Constructors
Methods