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

[Feature] Implemented Datetime object support #675

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

MannarAmuthan
Copy link
Contributor

@MannarAmuthan MannarAmuthan commented Oct 2, 2023

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:

  • Bug fix
  • [x ] New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Housekeeping:

  • [ x] Tests have been updated to reflect the changes done within this PR (if applicable).
  • [ x] Documentation has been updated to reflect the changes done within this PR (if applicable).

Added methods for datetime object

Constructors

  1. Datetime.new() -> Datetime
  2. Datetime.new(String, String) -> Datetime
  3. Datetime.new(Number) -> Datetime

Methods

  1. datetimeObj.strptime() -> Number
  2. datetimeObj.strftime(String) -> String

@MannarAmuthan MannarAmuthan mentioned this pull request Oct 2, 2023
1 task
@MannarAmuthan
Copy link
Contributor Author

MannarAmuthan commented Oct 2, 2023

Looks like there is some issue in handling gmt in windows, will fix it

Copy link
Member

@Jason2605 Jason2605 left a 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

Comment on lines 406 to 407
// defineNative(vm, &module->values, "now", nowNative);
// defineNative(vm, &module->values, "nowUTC", nowUTCNative);
Copy link
Member

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

Suggested change
// defineNative(vm, &module->values, "now", nowNative);
// defineNative(vm, &module->values, "nowUTC", nowUTCNative);

@@ -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();
Copy link
Member

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?

@Jason2605
Copy link
Member

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!

@Jason2605
Copy link
Member

Jason2605 commented Oct 5, 2023

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)

@MannarAmuthan
Copy link
Contributor Author

Certainly, could you provide some contract ideas that I can attempt to implement ?.
Because current methods such as datetimeObj.strptime, datetimeObj.strftime could not return Datetime object. Am I right?

@Jason2605
Copy link
Member

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:

Datetime.new(<values>); -> Datetime
Datetime.strptime(<string>, <string>); -> Datetime
Datetime.getTime(); -> Number
Datetime.strftime(<string>); -> String
Datetime.toString(); -> String (thinking do we need this if we have strftime actually? we could have this be strftime with no format specifier passed to it)


// Later down the line
Datetime.addMinutes(<number>); -> Datetime
Datetime.addHours(<number>); -> Datetime
...

@MannarAmuthan
Copy link
Contributor Author

Thanks for Suggestions,
I assume that we have two different types of methods/functions, one is on Datetime module, and another is on Datetime object itself.

Methods of Datetime module

Already there

Datetime.new(); -> Datetime(Object) , current time object
Datetime.newUTC(); -> Datetime(Object) , current time object

To implement/modify

Datetime.new(); -> Datetime (Object)
Does values means day,month,year, hour, etc ..?
Datetime.strptime(, ); -> Datetime(Object) , currently it returns string i guess

Methods of Datetime Object

Already there

dateTime(object).strftime() -> String , returns string

In future

dateTime(object).addMinutes(); -> Datetime(Object)
dateTime(object).addHours(); -> Datetime(Object)
...

@Jason2605
Copy link
Member

Jason2605 commented Oct 7, 2023

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

@Jason2605
Copy link
Member

No rush or anything @MannarAmuthan but just checking if you're still working on this?

@MannarAmuthan
Copy link
Contributor Author

Hi yes , I am planning to work on this in weekend

@MannarAmuthan
Copy link
Contributor Author

Hi @Jason2605 , Please look into it, and provide any suggestions or changes needed.

@Jason2605
Copy link
Member

Will take a look later today, thanks @MannarAmuthan

Copy link
Member

@Jason2605 Jason2605 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 this!! Few small comments really

UNUSED(args);

if (argCount != 0) {
runtimeError(vm, "nowNative() takes no arguments (%d given)", argCount);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runtimeError(vm, "nowNative() takes no arguments (%d given)", argCount);
runtimeError(vm, "now() takes no arguments (%d given)", argCount);

UNUSED(args);

if (argCount != 0) {
runtimeError(vm, "nowUTCNative() takes no arguments (%d given)", argCount);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runtimeError(vm, "nowUTCNative() takes no arguments (%d given)", argCount);
runtimeError(vm, "nowUTC() takes no arguments (%d given)", argCount);

UNUSED(args);

if (argCount != 0) {
runtimeError(vm, "nowUTCNative() takes no arguments (%d given)", argCount);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runtimeError(vm, "nowUTCNative() takes no arguments (%d given)", argCount);
runtimeError(vm, "newUTC() takes no arguments (%d given)", argCount);


if(argCount == 1) {
if (!IS_NUMBER(args[0])) {
runtimeError(vm, "new() takes 1 argument , must be a number");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runtimeError(vm, "new() takes 1 argument , must be a number");
runtimeError(vm, "new() argument must be a number");

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);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runtimeError(vm, "new() takes 0 or 1 argument, (%d given)", argCount);
runtimeError(vm, "new() takes 0 or 1 arguments (%d given)", argCount);

datetime.strftime("Today is %A"); // Today is Friday
```

Returns in default format when user defined format is not provided
Copy link
Member

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

time_t t = time(NULL);
struct tm tictoc;
char time[26];
char *queueString = malloc(sizeof(char) * 11);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
char *queueString = malloc(sizeof(char) * 11);
char *datetimeString = malloc(sizeof(char) * 11);

Copy link
Member

Choose a reason for hiding this comment

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

*And same below

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);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runtimeError(vm, "strftime() takes 0 or 1 argument (%d given)", argCount);
runtimeError(vm, "strftime() takes 0 or 1 arguments (%d given)", argCount);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These suggestions are done

@Jason2605
Copy link
Member

@briandowns Be good to get your thoughts on this too

@briandowns
Copy link
Contributor

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

@MannarAmuthan
Copy link
Contributor Author

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.

@Jason2605
Copy link
Member

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

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

@MannarAmuthan
Copy link
Contributor Author

MannarAmuthan commented Oct 28, 2023

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 {
long time; /* timestamp integer*/
int is_local;
} Datetime;

But I worry, that we need the capability to represent/create Datetime before 1970 also.
If you have any thoughts on this let me know.

@briandowns
Copy link
Contributor

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.

@Jason2605
Copy link
Member

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)

@briandowns
Copy link
Contributor

briandowns commented Oct 31, 2023

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.

@MannarAmuthan
Copy link
Contributor Author

Thanks for all suggestions, I am planning to work on this , will provide updates

@MannarAmuthan
Copy link
Contributor Author

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.

@Jason2605
Copy link
Member

Thanks for still working on this! Will be a really nice addition to get a more fully featured datetime library

@MannarAmuthan
Copy link
Contributor Author

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:
In Dictu, there is no named or keyword arguments for function calls, which restrict us to defining clear contract for timedelta object.

Approach 1: Constructing TimeDelta by dictionary

mydatetime = Datetime.new();

toAdd = Timedelta({
'days': 2,
'hours': 1
});  // Can have year , month, etc

mydatetime.add(toAdd); // Returns new datetime object

Pros

  • Can have more operations such as getting delta difference of two datetime objects, as delta object, etc.
  • Having clear difference with datetime object

Cons

  • API might looks slightly complex, especially introducing new object.
  • Constrcution of timedelta object looks little nasty

Approach 2: Avoid delta object

mydatetime = Datetime.new();
mydatetime.addDays(1);
mydatetime.addMinutes(2);
mydatetime.addYears(1); 
//etc

or

mydatetime = Datetime.new().addDays(1).addMinutes(2).subSeconds(1);

Pros

  • Very simple and minimal in terms of design

Cons

  • Limited functionality
  • Might lead to defining lot of methods

@Jason2605
Copy link
Member

Jason2605 commented Dec 11, 2023

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

@briandowns
Copy link
Contributor

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.

@Jason2605
Copy link
Member

Yeah when you run .add or .minus (or whatever we decide the public API to be) I would expect a new datetime object yeah!

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)

@briandowns
Copy link
Contributor

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.

@Jason2605
Copy link
Member

So you'd prefer methods rather than overloading the operators?

Makes sense, yeah I think im siding towards Duration at the moment

@briandowns
Copy link
Contributor

Yeah, for sure for methods over operators. Seems like the surface area for that is super small and still achieves the necessary functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Datetime
3 participants