Date time proposed improvements #7096

cooldome · 2018-01-17T00:16:08Z

After major rework of times module by GULF, I decided to give it a second chance and try to replace my hand written closed source Date module with standard lib times module.
I found that now times has all the functionality I need, but unfortunately external interface is such that I can't used it effectively. Coupling date and time together has its drawbacks. Time requires timezones and timezones requires reading external files that takes enormous amount time compared to rest of the calculations.

There no tests yet so it is work in progress, but I would like to start the discussion if anyone see major problems with the design.

Summary of changes:

New types: Date and EpochDate
DateTime is inherited from Date which helps sharing large part of the functionality.
Date has similar functionality to DateTime but much more efficient
weekday and yearday are now computed on the fly

GULPF · 2018-01-17T06:17:10Z

Looks great! Have you seen #6978? I think it makes some changes that are relevant to this PR.

I think instead of introducing separate procs like parseDate, date we should use generics (with overloads that assume DateTime):

proc now[D: Date|DateTime](): D
proc parse[D: Date|DateTime](value, format: string): D

The reason that I prefer that design is that I think it will be more intuitive when even more types are added (TimeOfDay and NaiveDateTime).

DateTime is inherited from Date which helps sharing large part of the functionality.

I disagree with this. When TimeOfDay is introduced DateTime can't inherit from it, even though it makes as much sense as inheriting from Date. It's better to let DateTime contain a Date and use DateTime|Date to share functionality.

weekday and yearday are now computed on the fly

Excellent. This will break code that tries to modify those fields, but IMO it's worth it.

GULPF · 2018-01-17T06:17:22Z

lib/pure/times.nim

    rtl, extern: "ntEqTime", tags: [], raises: [], noSideEffect, borrow.}
  ## Returns true if ``a == b``, that is if both times represent the same point in time.

+proc `-`*(a, b: EpochDate): int32 {.borrow.}


I think this should return the Duration type from #6978

As soon as Duration will appear in devel I can do that, otherwise it is problematic to test

GULPF · 2018-01-17T06:18:31Z

lib/pure/times.nim

+proc date*(monthday: MonthdayRange, month: Month, year: int): Date =
+  ## Create a new ``Date``
+  result = Date(monthday: monthday, month: month, year: year)
+  assertValidDate result


This proc should be named initDate.

GULPF · 2018-01-17T06:23:59Z

lib/pure/times.nim

+proc date*(s: string): Date = 
+  ## Parse `s` date string using ISO format
+  runnableExamples:
+    date"2019-03-21"


Example should verify the result of date"2019-03-21", this will likely complain about missing discard.

I'm not really sure about this proc. If the use case is initialization, then IMO it should limit the argument to string literals so that parseDate(str, "yyyy-MM-dd") must be used when parsing input. A proc named date that does parsing seems a bit unclear.

Removed this proc as requested. I can add date alias locally in my project.
I understand that it is deviation from the existing API.
However, it is super convenient to have a single function date that does all ways of date creation.

date"2019-02-03" date(2019, mFeb, 3) date(2019, 02, 03) date() current date date(x), where x can be EpochDate, Time, DateTime

So all you need to remember is the word date and you will be just fine with stdlib.
You can't do that effectively with DateTime as it has far too many members to keep specifying them one by one.

Please vote if you want date function back

GULPF · 2018-01-17T06:28:24Z

lib/pure/times.nim

+proc toEpochDate*(time: Time): EpochDate =
+  EpochDate(time.int64 div secondsInDay)
+
+proc toEpochDate*(epoch_days: int32): EpochDate =


Should be epochDays instead of epoch_days

GULPF · 2018-01-17T06:38:27Z

lib/pure/times.nim

+    let dt = date()
+    result.monthday = dt.monthday
+    result.month = dt.month
+    result.year = dt.year


This is not correct? It will overwrite all of monthday, month and year even if some of them might have been parsed.

Previous behavior was to call now() at the begging of parse function unconditionally. It is tearing my heart apart when I see a such wasteful behavior, now() is not cheap and it is 95% probably that you will find a date in the string, so results of now() call will be overridden and wasted. The idea is to call now() only it date was not found in the string

GULPF · 2018-01-17T07:03:22Z

lib/pure/times.nim

+  assertValidDate d
+  toEpochDate(d.year, d.month.int, d.monthday.int)
+
+proc date*(epochdate: EpochDate): Date =


I dislike the naming of this. I think fromEpochDate better matches the rest of the API in the times module.

done for now, unless idea of single entry date will be supported

GULPF · 2018-01-17T07:09:06Z

lib/pure/times.nim

+proc yearday*(d: Date): YeardayRange {.tags: [], raises: [], benign .} =
+  ## Returns the day of the year since January 1,
+  ## in the range 0 to 365.
+  assertValidDate d


assertValidDate shouldn't be needed here. When taking proper types like Date or DateTime as input they can be assumed to be valid.

GULPF · 2018-01-17T07:15:43Z

lib/pure/times.nim

+proc toEpochDate*(epoch_days: int32): EpochDate =
+  epoch_days.EpochDate
+
+proc toEpochDate(year, month, day: int): EpochDate =


Use the argument ordering day, month, year instead to be consistent with rest of the times module.

GULPF · 2018-01-17T17:11:16Z

After thinking about this some more, I don't think my idea of using generics for now/parse is good. parseX is how most parse procs are named in the stdlib (parseJson, parseInt etc) so parseDate proc is a good name. However, I still dislike the date proc which returns the current date. I'd rather add a proc for converting DateTime to Date so that now().date can be used instead.

dom96 · 2018-01-17T18:14:25Z

If you're using inheritance then I don't think generics are a good idea.

Why the need for EpochDate?

cooldome · 2018-01-20T22:13:57Z

@GULPF
comment on

However, I still dislike the date proc which returns the current date. I'd rather add a proc for converting DateTime to Date so that now().date can be used instead"

now() is producing DateTime by converting Time to DateTime. Time->DateTime->Date is far to inefficient, it should be Time->Date . I have changed the name to nowDate to closer match existing API. I hope it is ok this way

cooldome · 2018-01-20T22:32:56Z

@dom96

EpochDate naturally exists. It is in the heart of every date algorithm. The only question if you want to expose it or not.

For example how is any DateTime gets produced in reality. It is a sequence of conversions:
EpochTime -> EpochDate + remainder -> Date + remainder -> Date + Time -> DateTime
Step from remainder to Time requires timezone. I want user to be able to use parts of this pipeline if he wants to. I have concentrated on Date part, having time part separately would also help. Many date operations are more efficient on EpochDate then on Date. As a side note, I have a generic type in my code: Interval[a, b: static[EpochDate], T] = distinct T, given that EpochDate is as efficient as int, it helps compilation efficiency

GULPF · 2018-01-21T09:46:26Z

lib/pure/times.nim

+    let dt = nowDate()
+    result.monthday = dt.monthday
+    result.month = dt.month
+    result.year = dt.year


Take his example: parse(str, "MM/dd"). It should parse the month and monthday and use the current year. This code however will overwrite all of the date fields with the current ones. The code needs to check if each individual field is missing before overwritting:

if ptMonthday notin found_tokens: result.monthday = dt.monthday

GULPF · 2018-01-21T10:00:49Z

lib/pure/times.nim

+    $d.year & "-" & $ord(d.month) & "-" & $d.monthday & " is not a valid date"
+
+proc toEpochDate*(time: Time): EpochDate =
+  EpochDate(time.int64 div secondsInDay)


I don't think this will be correct for times before 1970-01-01, e.g a time of -1 will give the epoch date 0, but it should be -1.

fixed, should work now

GULPF · 2018-01-21T10:10:54Z

now() is producing DateTime by converting Time to DateTime. Time->DateTime->Date is far to inefficient, it should be Time->Date . I have changed the name to nowDate to closer match existing API. I hope it is ok this way

The reason that nowDate is fast is because it doesn't consider timezones, but it should consider timezones. It would be very confusing if now() and nowDate() gives different dates. This also means that the change to getDateStr changes behavior: previously it returned the local date, now it returns the UTC date.

If we need a fast and short way to get the current UTC day, would introducing a nowUtc proc be enough, so that nowUtc().date can be used? Allowing getTime().toDate is another alternative.

If we really need a single proc for getting the UTC date, then it should be named nowDateUtc or similar, but I think it would be overkill. It would require us to have two nowX procs for every introduced type to be consistent, resulting in 8 different nowX procs when TimeOfDay and NaiveDateTime is introduced.

getTime().toEpochDate().fromEpochDate()

After reading this I think I was wrong to say that fromEpochDate is a good name for the proc, that code looks incredible confusing... I forgot that the new API have multiple variants of toEpochDate. The EpochDate->Date proc should probably just be called toDate. Sorry for the bikeshedding

cooldome · 2018-01-21T12:51:07Z

@GULPF
Are you sure that timezone changes can cause date differences, not just time of the day differences?
It is news to me. Googled it quickly, can't find any evidence of that.

GULPF · 2018-01-21T13:10:18Z

@cooldome
Yes, here is an example output of zdump that I just ran:

$ zdump Asia/Anadyr
Asia/Anadyr  Mon Jan 22 01:06:19 2018 +12
$ zdump Europe/Stockholm
Europe/Stockholm  Sun Jan 21 14:06:29 2018 CET

As you can see, the date in Asia/Anadyr is currently Jan 22, while in Europe/Stockholm it's currently Jan 21. What else would happen when for example UTC is at 23:00 and you're in a timezone that is more than one hour ahead of UTC?

cooldome · 2018-01-21T17:46:56Z

@GULPF:
I have renamed fromEpochDate to toDate. toDate is definitely better.
Thanks for your patience and comprehensive description of the nowDate() pitfalls.
nowDate is now removed as it causes more problems then solves, those who want quick UTC date can use getTime.toEpochDate.toDate

johannschopplich · 2018-02-27T07:59:06Z

I catched myself in the act again checking for an update on this PR. I use the times module extensively, but need improvements. This is what I'm waiting for. Great work—can't wait for an update / merge on this. No joke.

Araq · 2018-02-27T08:58:50Z

@jschopplich Will have to a wait a little longer then, this won't make it into the next release, sorry.

Araq · 2018-02-27T08:59:28Z

nowDate is now removed as it causes more problems then solves, those who want quick UTC date can use getTime.toEpochDate.toDate

That sounds wrong. If so, why is nowDate not an alias for getTime.toEpochDate.toDate?

GULPF · 2018-02-27T09:08:21Z

@Araq
Read the discussion above. getTime.toEpochDate.toDate uses UTC, but now() uses the local timezone. If there should be a nowDate, it should use the local timezone as well.

johannschopplich · 2018-02-27T10:12:39Z

Will have to a wait a little longer then, this won't make it into the next release, sorry.

Alright. 😐 A proper times module is the one thing missing to put Nim above other languages in my production use cases. I will wait then—thank you @GULPF and @cooldome for the already merged improvements since v0.17.2. Keep it up! 👍

skilchen · 2018-03-31T03:21:36Z

Please don't introduce the inheritance relation between Date and DateTime.
They are simply different scales on an abstract notion of time (similar to km and m for distances, but nobody thinks that a meter inherits something from a kilometer). Do you know any other Date/Time library where DateTime inherits from Date?

Consider what happens if you want to implement arithmetic on Dates. Lets say you implement subtraction of date1 from date2. With your inheritance relation this becomes also valid for DateTimes but gives undesired results. How would you then implement subtraction specifically for DateTimes?

Given that @GULPF will surely convince you, that you have to take timezones into account also for dates, you actually gain very little from a separate Date type.

GULPF · 2018-04-02T15:52:29Z

Given that @GULPF will surely convince you, that you have to take timezones into account also for dates, you actually gain very little from a separate Date type.

I'm not sure what you mean by this since the Date type doesn't have a timezone, but I agree that DateTime shouldn't inherit from Date.

skilchen · 2018-04-03T16:50:20Z

Given that @GULPF will surely convince you, that you have to take timezones into account also for dates, you actually gain very little from a separate Date type.

sorry this was a failed attempt to argue from authority ...

if dates represent some number of days since an epoch and days are defined as in UTC (based on solar mean time), then it seems clear to me, that at least the question "what date is it today?" depends just as much on the location on earth where you are located as the question "what time is it now()?".

GULPF · 2018-04-03T17:55:48Z

The question "what date is it today" requires knowledge of timezones, but not questions like "what date was I born?" or "what date is next Christmas?". The main purposes of the Date type is to track abstract dates that doesn't represent any specific point in time, which it can't do if it represents some number of days since an epoch.

The fact that this PR adds the type EpochDate muddies the waters a bit. I dislike the type because it's not clear what it represents. Is equivalent to Date but stored as an int32, or is it equivalent to Time but with days as resolution? If it was an internal type it wouldn't really matter, but I think as an exported type it's not clear enough what it actually represents.

skilchen · 2018-04-06T00:04:12Z

my humble opinion: close this PR and wait until @GULPF comes up with a separate Date type

Araq · 2018-04-13T05:43:59Z

I think the PR queue for times.nim is now otherwise empty. Time to revisit this.

cooldome · 2018-04-26T22:27:19Z

Was not paying attention what is was happening with this PR. Will get back to it soon

Araq · 2018-08-31T10:25:53Z

A friendly reminder. Ping @cooldome

cooldome · 2018-09-07T20:28:24Z

ah, yes. In next release then, all my energy is drained by #8489 which a blocker for my project

Araq · 2018-12-21T17:02:40Z

No progress. Please reopen when you get back to it.

timotheecour · 2018-12-21T21:20:33Z

@Araq that's what the labelS: Waiting on author is for :-) (even if you close it, at least adding that label helps)
helps triaging since there's good stuff here that'd be a shame to loose in case either author or someone else wants to take over

cooldome added 3 commits January 9, 2018 23:25

Date_time progress

a77b943

Make sure all tests are passing

be9855a

Merge two format functions

bf52f46

GULPF reviewed Jan 17, 2018

View reviewed changes

This was referenced Jan 18, 2018

Add getDayOfWeek from epoch date #7020

Closed

times.parse performance improvement for dates #7007

Closed

cooldome added 4 commits January 19, 2018 20:48

Bug fix

b5f4d35

Further progress on Date

1e132cf

Merge remote-tracking branch 'upstream/devel' into date_time

70b908a

incorporate feedback

3263ac1

Add first tests

f0f0526

GULPF reviewed Jan 21, 2018

View reviewed changes

cooldome added 2 commits January 21, 2018 15:39

extra fixes

01a2b54

remove nowDate

4b077a6

small improvement

3b52afd

Araq closed this Dec 21, 2018

timotheecour added the Waiting on Author label Dec 21, 2018

Uh oh!

Date time proposed improvements #7096

Date time proposed improvements #7096

Uh oh!

Conversation

cooldome commented Jan 17, 2018

Uh oh!

GULPF commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cooldome Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GULPF Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GULPF Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GULPF commented Jan 17, 2018

Uh oh!

dom96 commented Jan 17, 2018

Uh oh!

cooldome commented Jan 20, 2018

Uh oh!

cooldome commented Jan 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GULPF commented Jan 21, 2018

Uh oh!

cooldome commented Jan 21, 2018

Uh oh!

GULPF commented Jan 21, 2018

Uh oh!

cooldome commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johannschopplich commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Araq commented Feb 27, 2018

Uh oh!

Araq commented Feb 27, 2018

cooldome Jan 20, 2018 •

edited

Loading

GULPF Jan 17, 2018 •

edited

Loading

GULPF Jan 17, 2018 •

edited

Loading

cooldome commented Jan 21, 2018 •

edited

Loading

johannschopplich commented Feb 27, 2018 •

edited

Loading

johannschopplich commented Feb 27, 2018 •

edited

Loading

timotheecour commented Dec 21, 2018 •

edited

Loading