possible regression in `initInterval` of the `times` module

[Vindaar]

I have a small program to extract time information from some data. I recompiled it using the current 0.18.1 devel branch and suddenly my output is 0 days, 0 hours and 0 minutes instead of a reasonable time interval.
The code essentially boils down to the following example from the times docs:
https://play.nim-lang.org/?gist=ebc0c0a8f45eb2a31e61ce54262afb9a

import times

let a = fromSeconds(1_000_000_000)
let b = fromSeconds(1_500_000_000)
echo initInterval(seconds=int(b - a))

where the expected output should be

(milliseconds: 0, seconds: 20, minutes: 53, hours: 0, days: 5787, months: 0, years: 0)

but the actual output is:

(milliseconds: 0, seconds: 500000000, minutes: 0, hours: 0, days: 0, months: 0, years: 0)

The input to initInterval is not used anymore to populate all fields (up to days at least) anymore. Instead each field is only set to the corresponding argument.

The implementation of initInterval in fact is:

proc initInterval*(milliseconds, seconds, minutes, hours, days, months,
                   years: int = 0): TimeInterval =
  result.milliseconds = milliseconds
  result.seconds = seconds
  result.minutes = minutes
  result.hours = hours
  result.days = days
  result.months = months
  result.years = years

where for example the implementation in 0.17.2 was

proc initInterval*(milliseconds, seconds, minutes, hours, days, months,
                   years: int = 0): TimeInterval =
  var carryO = 0
  result.milliseconds = `mod`(milliseconds, 1000)
  carryO = `div`(milliseconds, 1000)
  result.seconds = `mod`(carryO + seconds, 60)
  carryO = `div`(carryO + seconds, 60)
  result.minutes = `mod`(carryO + minutes, 60)
  carryO = `div`(carryO + minutes, 60)
  result.hours = `mod`(carryO + hours, 24)
  carryO = `div`(carryO + hours, 24)
  result.days = carryO + days

  result.months = `mod`(months, 12)
  carryO = `div`(months, 12)
  result.years = carryO + years

In case this is expected behavior now and the user is supposed to use a different way to get the old result, the docs should reflect that. However, I can't even find a convenient way to get the number of days, hours etc. of a time interval.

[GULPF]

This was missing in the changelog, but it was an intended change made in #6552. I'll quote my reasoning here:

My last commit makes another breaking change: TimeInterval no longer normalizes the time units, so that f.ex 24.hours doesn't result in a TimeInterval with one day.

Motivation

Because of the fix for #6465, a day is no longer always 24 hours, which means that TimeInterval can't normalize 24 hours as 1 day. Furthermore, if TimeInterval is used with a calendar that supports leap seconds (currently not possible since the times module doesn't have the concept of calendars, but it might it the future), then it can't normalize 60 seconds as 1 minute either.

This would mean that the only normalization that can be done is milliseconds => seconds, minutes => hours and months => years. Because I think it would be highly confusing to only perform normalization in these cases, I think it's better to do no normalization at all.

With this reasoning it also makes sense that there is no way to get the normalized number of days/months/years from a TimeInterval, since it's not clear how long those time units are. This new design of TimeInterval is similiar to Javas Period class. My current PR (#6978) adds an equivalent to Javas Duration class. The problem with the previous design in the times module was that TimeInterval tried to represent both these things (Period/Duration).

I can make a PR to improve the docs a bit.

[Vindaar]

Ahh, I see. Thanks for the clarification! I'll take a look at your PR.

[narimiran]

I can make a PR to improve the docs a bit.

Please do, so we can close this.