Feature/times fixup

[survivorm]

No description provided.

[survivorm]

Need to review carefully, submitted to be sure to not forget

[GULPF]

Thanks for taking the time to do this :) I'll do a proper review later, but one thing I noticed now: that TimeInterval doesn't do any normalization is by design and should not change. Note that initTimeInterval(days = 1) is not the same thing as initTimeInterval(hours = 24), and that they actually behave differently when added to a DateTime. Sadly this is not properly documented yet.

For some more information about why it's designed this way, take a look at the documentation for the Java classes Duration and Period, which are similiar to our Duration and TimeInterval.

[survivorm]

Interesting. Then isn't it a proper thing to divide it into TimeInterval and NormalizedTimeInterval?
Cause sometimes it's much more convenient to have both options. May be it should work not for all options, but ability to convert TimeInterval to it's Normalized version seems natural

[GULPF]

NormalizedTimeInterval is almost what Duration is, so I don't think an additional type is needed. Having two different types for this is already complex enough, I would have preferred to just have Duration. Do you have a use case for NormalizedTimeInterval that isn't covered by TimeInterval/Duration?

[survivorm]

Okay, valid point. The case not covered by Duration is quite simple, really. I want to be able to get representation in all units starting from greater to lesser like Duration -> NormalizedTimeInterval (it might be time interval, in essence, but it will have years, months, weeks, days and etc. It's not for computing, more like human-representetion of duration)

[survivorm]

And, of cause, if the non-normalization of TI is by design, I want to have a valid point WHY. In essence, I don't see a difference in TI(days=1) and TI(hours=24). They shoud be the same in my opinion, if not, i'd like to see why

[andreaferretti]

I am copying from the documentation of Period linked above:

For example, consider adding a period of one day and a duration of one day to 18:00 on the evening before a daylight savings gap. The Period will add the conceptual day and result in a ZonedDateTime at 18:00 the following day. By contrast, the Duration will add exactly 24 hours, resulting in a ZonedDateTime at 19:00 the following day (assuming a one hour DST gap).

[survivorm]

hm. Thanks for clarification. But that's one case and i'm not sure it's very useful for comparisons. Like, i think almost anyone seeing comparison TI(hours=24) == TI(days=1) are expecting true

[survivorm]

Ah. I see the human-readable duration's been made while I've been working on my branch. But i'm a little against that it's string-only. I'd better have it as a Duration -> tuple or Duration -> TimeInterval conversion (i'm not quite sure latter will be valid, considering their difference), than used in $

[andreaferretti]

i think almost anyone seeing comparison TI(hours=24) == TI(days=1) are expecting true

I have been trained to never expect something like that from dates and times. A day is not always 24 hours, a year is not always 365 days, a minute is not always 60 seconds, timezone changes are sometimes 5 minutes and 52 seconds, and so on...

[GULPF]

I think this use case should be supported by implementing the equivalent of Javas Period.between. The Duration to TimeInterval conversion could then be performed like this: between(now(), now() + dur).

[survivorm]

Maybe (and most likely) you're right. But than it must be as explicit as it may be. Than we must add this to any TimeInterval docstring available. Everythere, because it's a racks almost anyone is bound to step on.

So, I think my goal for now is:

1. Drop the commit with TimeInterval normalization
2. Add comments about:

• TimeInterval being NOT NORMALIZED and why.
• TimeInterval comparisons limitations

3. Add helper function to convert Duration to NormalizedTimeInterval (?). Use it in $(Duration)

---

edit

Point 3 may be made via between function, as suggested by GULPF. But there's a but here - should it use a strait ariths like between(now(), now() + dur) is the same for any now()? As far as I understand, it may differ sumetimes, like in the example with winter/summer clock change.

Any more suggestions/comments?

[GULPF]

1. Drop the commit with TimeInterval normalization
2. Add comments about:
   `TimeInterval being NOT NORMALIZED and why.
   TimeInterval comparisons limitations

Sounds great

Add helper function to convert Duration to NormalizedTimeInterval (?).

Please implement it as between like I described above:

proc between(startDt, endDt: DateTime): TimeInterval

Use it in $(Duration)

Don't do this though. $dur should never print the units year and month, since that's not what it represents. Unlike TimeInterval, Duration is not a calendar based duration, so initDuration(hours = 24) == initDuration(days = 1) is actually true (just like Duration in Java).

[survivorm]

GULPF understood. But read edit in my last comment and confirm it's validity, please. Then i'll continue working on this PR.

[GULPF]

Point 3 may be made via between function, as suggested by GULPF. But there's a but here - should it use a strait ariths like between(now(), now() + dur) is the same for any now()? As far as I understand, it may differ sumetimes, like in the example with winter/summer clock change.

I don't fully understand the question. between(now(), now() + dur) will give different result depending on the current time & timezone. E.g there's 0 months between 2018/01/01 and 2018/01/29, but 1 month between 2018/02/01 and 2018/03/01, even though the actual duration between them is the same.

[survivorm]

@GULPF Yeah, that's it. But i mean something else. I want to get something human-readable (and manipulable, still) in format of at least (, seconds, minutes, hours). May be even months and years, but that's a little less convenient as you're right on that that duration heavily depends on the starting point.
So, i think the end will be helper fun on Duration -> DurationTuple (weeks, days, hours, minutes, etc.) - which are always the same and will be used for $(Duration) and between, which may result in different output given the same duration, which user may use in any way he like

[GULPF]

So, i think the end will be helper fun on Duration -> DurationTuple (weeks, days, hours, minutes, etc.) - which are always the same and will be used for $(Duration) and between, which may result in different output given the same duration, which user may use in any way he like

This sounds good :)

[skilchen]

In several runnableExamples you are assuming that the current utcOffset is the same as it was on 1970-01-01. In many timezones this is not true, especially in the ones using some kind of DST.
Example:

proc format*(time: Time, f: string): string {.tags: [].} =
  ## converts a `Time` value to a string representation. It will use the local
  ## time zone and use the format from ``format(dt: DateTime, f: string)``.
  runnableExamples:
    let tm = fromUnix(0) + now().utcOffset.seconds
    doAssert format(tm, "yyyy-MM-dd'T'HH:mm:ss") == "1970-01-01T00:00:00"
  time.local.format(f)

Many people living in areas where DST is currently in effect will see an AssertionError here, if they try to run nim doc times.

[survivorm]

@skilchen okay, i'll look into it a little more closely

[survivorm]

between made me climbing the walls, almost literally. It's brain damaging and i still do not like the code, i'm pretty sure it can be simplified. But it's working, hopefully

[skilchen]

Your commendable attempt to solve the relatively difficult task to write between needs some improvements:

import times
var a = initDateTime(year = 2018, month = Month(3), monthday = 25, 
                     hour = 0, minute = 59, second = 59, nanosecond = 999_999_999,
                     zone = utc())
var b = initDateTime(year = 2018, month = Month(3), monthday = 25, 
                     hour = 1, minute =  0, second =  0, nanosecond = 0,
                     zone = utc())
echo between(a, b)

gives:

times.nim(1322)          between
system.nim(2844)         sysFatal
Error: unhandled exception: over- or underflow [OverflowError]

You are trying to set second = -1 in the result. Maybe you can simply turn the overflowchecks off, otherwise you have to bubble-up these overflows from nanoseconds to years...

Now in the local timezone of one of the european countries (eg. Europe/Paris) where the switch to DST happened between 00:59:59 and 01:00:00 UTC on 2018-03-25 and setting second and minute to 1 to avoid the overflows from above:

import times
var a = initDateTime(year = 2018, month = Month(3), monthday = 25, 
                     hour = 0, minute = 59, second = 59, nanosecond = 999_999_999,
                     zone = utc()).local
var b = initDateTime(year = 2018, month = Month(3), monthday = 25, 
                     hour = 1, minute =  1, second =  1, nanosecond = 0,
                     zone = utc()).local
var ti = between(a, b)
echo ti

gives:

(nanoseconds: 1, microseconds: 0, milliseconds: 0, seconds: 1, minutes: 1, hours: 23, days: 27, weeks: 0, months: 11, years: -1

which is wrong even when considering the fact that there are probably infinitely many ways to express a time interval. A correctnes test would be: a + ti == b which is not true in this case.
This is due to your attempt to deal with the utcOffsets. It would be better to simply convert both DateTimes to UTC before performing any calculations.

A minor issue: when handling the seconds you have a copy/paste error in your code (both in the positive and the negative branch):

    #Seconds
    if end_dt_offseted.second < start_dt.second:
      result.seconds = end_dt_offseted.nanosecond.int() +
        convert(Minutes, Seconds, 1) - start_dt.second.int()
      end_dt_offseted.minute -= 1
    else:
      result.seconds = end_dt_offseted.second.int() -
        start_dt.second.int()

the line result.seconds = end_dt_offseted.nanosecond.int() +should be result.seconds = end_dt_offseted.second.int() +

My own attempt to write a between procedure, looks like:

from math import floor

proc floorDiv*[T, U](m: T, n:U): int =
  ## Return the whole part of m/n rounded towards negative infinity.
  ## (also known as "floor division")
  return int(floor(m.float / n.float))

proc modulo*[T, U](x: T, y: U): U =
  ## The modulo operation using floor division
  ##
  ## x - floor(x / y) * y
  return x.U - floorDiv(x.U, y.U).U * y

template toEpochDays(dt: DateTime): int =
  toEpochDay(dt.monthday, dt.month, dt.year).int

proc between_b*(dt1, dt2: DateTime): TimeInterval =
  ## calculate the `TimeInterval` between two `DateTime`
  ## a loopless implementation inspired in the date part
  ## by the until Method of the new java.time.LocalDate class
  ##

  var dt1 = dt1.utc()
  var dt2 = dt2.utc()
  var sign = 1
  if dt2 < dt1:
    swap(dt1, dt2)
    sign = -1

  let ts1 = initTime(unix = dt1.hour * 3600 + dt1.minute * 60 + dt1.second,
                     nanoseconds = dt1.nanosecond)
  let ts2 = initTime(unix = dt2.hour * 3600 + dt2.minute * 60 + dt2.second,
                     nanoseconds = dt2.nanosecond)
  let difftime = ts2 - ts1
  let diffdays = floorDiv(difftime.seconds, 86400)
  var diffseconds = int(difftime.seconds - 86400 * diffdays)
  let diffhours = floorDiv(diffseconds, 3600)
  let diffminutes = floorDiv(diffseconds - 3600 * diffhours, 60)
  diffseconds = diffseconds - 3600 * diffhours - 60 * diffminutes

  dt2 = dt2 + initTimeInterval(days = diffdays)

  var totalMonths = dt2.year * 12 - dt1.year * 12 + ord(dt2.month) - ord(dt1.month)
  var days = dt2.monthday - dt1.monthday
  if (totalMonths > 0 and days < 0):
    totalMonths.dec
    let tmpDate = dt1 + initTimeInterval(months = totalMonths)
    days = int(dt2.toEpochDays() - tmpDate.toEpochDays())
  elif (totalMonths < 0 and days > 0):
    totalMonths.inc
    days = days - getDaysInMonth(year = dt2.year, month = dt2.month) + 1
  let years = totalMonths div 12
  let months = totalMonths mod 12

  return initTimeInterval(years = sign * years,
                          months = sign * months,
                          days = sign * days,
                          hours = sign * diffhours,
                          minutes = sign * diffminutes,
                          seconds = sign * diffseconds,
                          nanoseconds = sign * difftime.nanoseconds)

Maybe you get some inspiration from my attempt which most likely could also be improved. You should be able to simply insert my code at the bottom of times.nim to play with it. (It uses toEpochDay which times.nim doesn't export, therefore the need to insert the code into times.nim).

[GULPF]

Agreed that between is infuriating to implement...

I don't think between should convert to UTC, because it might affect the result and could cause a + between(a, b) == b to fail (e.g if the UTC conversion causes the dates to change). The difference in utcOffset can just be regarded as an additional difference in seconds, so I don't think it's impossible to support it.

However, between should probably enforce that the inputs at least have the same timezone since the operation doesn't make any sense for different timezones, and it will probably be incredible hard to support that anyway.

I think @skilchen's version is nearly correct, but it fails to handle this case:

let dt1 = initDateTime(10, mJan, 2018, 13, 00, 00, utc())
let dt2 = initDateTime(11, mJan, 2018, 12, 00, 00, utc())
echo between(dt1, dt2) # hours = 23, days = 1

Also, I think the result should be normalized as much as possible. This would mean that nanoseconds should be split into micro-/milli-/nanoseconds and that days should be split into weeks/days.

[survivorm]

@GULPF can you please give an example of the situation there conversion to unicode breaks the work?
And i'll post my fixed version in a few minutes

[survivorm]

Posted. Hopefully this works, though i'm still not sure about edge scenarios. Not sure should split into weeks, however, but the idea with ns, mks and ms is sound

[survivorm]

@dom96 Still waiting your comments on mutator funcs

[survivorm]

Ah, i see the problem with mutators, never mind

[survivorm]

Hopefully fixed the mutators problem. But still don't like it, but my suggestion on {default} pragma for the generics to be used in choice only AFTER anything else failed (no more specific proc) has been turned down, AFAIK

[skilchen]

i would prefer something like:

type TimesTypes = DateTime | Time | Duration | TimeInterval
proc `+=`*[T: TimesTypes, U](a: var T, b: U) =
  a = a + b

proc `-=`*[T: TimesTypes, U](a: var T, b: U) =
  a = a - b

proc `*=`*[T: TimesTypes, U](a: var T, b: U) =
  a = a * b

[dom96]

I would prefer to move these into a separate PR so that we can discuss them there. Like I mentioned I think we need a generic solution to this, if we simply merge this now we will end up forgetting.

[survivorm]

@skilchen Why only T is TimesTypes? Otherwise, great improvement.
@dom96 ok, i'll do it

[survivorm]

#7806
@skilchen
@dom96

[skilchen]

What about a stringification procedure for TimeIntervals?
Maybe something like:

proc `$`*(ti: TimeInterval): string =
  template addSep() =
    if len(result) != 0:
      result.add(", ")
  template addUnit(value: SomeNumber, unit: string) =
    result.add($value)
    result.add(" ")
    result.add(unit)
    if abs(value) != 1:
      result.add("s")

  result = ""
  if ti.years != 0:
    addUnit(ti.years, "year")
  if ti.months != 0:
    addSep()
    addUnit(ti.months, "month")
  if ti.weeks != 0:
    addSep()
    addUnit(ti.weeks, "week")
  if ti.days != 0:
    addSep()
    addunit(ti.days, "day")
  if ti.hours != 0:
    addSep()
    addUnit(ti.hours, "hour")
  if ti.minutes != 0:
    addSep()
    addUnit(ti.minutes, "minute")
  if ti.seconds != 0:
    addSep()
    addUnit(ti.seconds, "second")
  if ti.milliseconds != 0:
    addSep()
    addUnit(ti.milliseconds, "millisecond")
  if ti.microseconds != 0:
    addSep()
    addUnit(ti.microseconds, "microsecond")
  if ti.nanoseconds != 0:
    addSep()
    addUnit(ti.nanoseconds, "nanosecond")

[skilchen]

Why not add the badly missing feature to format/parse fractional seconds in this PR?

My proposals are:
for parseToken:

  of "f", "ff", "fff":
    var numStr = ""
    let n = parseWhile(value[j..len(value) - 1], numStr, {'0'..'9'})
    dt.nanosecond = parseInt(numStr) * (10 ^ (9 - n))
    j += n

and for formatToken:

  of "f":
    buf.add(intToStr(convert(Nanoseconds, Milliseconds, dt.nanosecond), 3))
  of "ff":
    buf.add(intToStr(convert(Nanoseconds, Microseconds, dt.nanosecond), 6))
  of "fff":
    buf.add(intToStr(dt.nanosecond, 9))

Personally i would also change the format String in the default DateTime stringification from:

result = format(dt, "yyyy-MM-dd'T'HH:mm:sszzz")

to

result = format(dt, "yyyy-MM-dd'T'HH:mm:ss'.'fzzz")

but that might be too controversial.

[dom96]

I would prefer to move these into a separate PR so that we can discuss them there. Like I mentioned I think we need a generic solution to this, if we simply merge this now we will end up forgetting.

[dom96]

This doesn't fix my concern. The default is still local and that is a problem. Taking this conscious explicit choice away from the developer is a bad idea.

[survivorm]

Okay. Looks like i'm done with the improvements, except separate pr on generics

[skilchen]

Thanks @survivorm for the implementation of my proposals! Now lets see, what the experts think ...

It gets somewhat difficult to keep the overview...
I think you may have missed my comment on this issue:

echo initDuration(days = -1)

now gives (again):
-23 hours, -59 minutes, -59 seconds, and -1000 milliseconds

What you had before in toDurationParts, was better.

  # Ensure the same sign for seconds and nanoseconds
  if remS < 0 and remNs > 0:
    remNs -= convert(Seconds, Nanoseconds, 1)
    remS.inc 1

what is indeed not needed if nanoseconds can't be negative is the else part you had there before.

[survivorm]

@skilchen Thanks. Yeah, i've missed it, thanks for rememberance. Will fix

[survivorm]

@dom96 waiting for your opinion on the latest additions

[survivorm]

@dom96 Ping. Are you still busy?