Add Week-Of-Year Implementation to Times Module

[capocasa]

ISO Week-of-year implementation for the times module using algorithm from wikipedia, which is in turn from Van Gent, Math Department of Uttrecht University. Previously referenced on the forum, used for a year in production with no issues. Fairly comprehensive unit tests. Performance should be ok, additional bit twiddling possible but probably won't be needed by anyone. Nimble package also possible but very small / seems to fit better here. Feedback welcome.

[juancarlospaco]

Use Natural and/or document that the argument must be positive, same with other proc.
Also needs {.since.} and changelog entry.

[capocasa]

Thanks for the help!

[capocasa]

@juancarlospaco > Use Natural

Added unit tests that demonstrate support for iron age dates instead (use case for negative integer)

Edit: Added {.since.} and changelog entry, I believe specified requirements are now complete

[timotheecour]

why do we need this overload? just use nanosecond: NanosecondRange = 0 above ?

[konsumlamm]

We already have two other initDateTime procs anyway, one of which is unnecessary, I assume @capocasa copied that from there?

[capocasa]

I did it that way for consistency with the existing API.

The alias proc isn't really unnecessary, it allows you to create a datetime by positional arguments with a time zone without specifying nanoseconds. I assume the original author concluded time zones were a more common use case than nanoseconds, and I agree. But even if you don't, having to remember which kind of initDateTime allows omitting nanoseconds with a time zone would suck, so the alias should stay for consistency's sake, expectations are there, too late to change them.

Edit: The correct way to add only one weekyear initDateTime seems to me to be to deprecate one of the existing ones, but I think it would better to add both and then potentially deprecate one of each later to decouple the deprecation discussion from adding week-of-year.

[timotheecour]

we shouldn't aggravate the problem, let's just introduce 1 overload, not 2

proc initDateTime*(weekday: WeekDay, yearweek: YearweekRange, year: int,
                   hour: HourRange, minute: MinuteRange, second: SecondRange,
                   nanosecond: NanosecondRange,
                   zone: Timezone = local()): DateTime {.since: (1, 5).} =

in future work, we can discuss whether to deprecate the pre-existing

proc initDateTime(monthday: MonthdayRange; month: Month; year: int;
                  hour: HourRange; minute: MinuteRange; second: SecondRange;
                  zone: Timezone = local())

and make nanoseconds an option param for

proc initDateTime(monthday: MonthdayRange; month: Month; year: int;
                  hour: HourRange; minute: MinuteRange; second: SecondRange;
                  nanosecond: NanosecondRange; zone: Timezone = local())

[capocasa]

I believe I have addressed all currently raised issues, please let me know if further improvements are in order. Thanks for all the input!

[timotheecour]

CI fails, you can run nim doc --lib:lib lib/pure/times.nim locally before pushing to avoid having to debug via CI

[timotheecour]

also add:

  block: # getWeeksInYear
    # add a few tests here

[capocasa]

Good call, caught a bug with negative years

[GULPF]

A common gotcha for ISO week dates is that the year numbering is not the same as in the gregorian calendar. From the Wikipedia article:

The year number of the ISO week very often differs from the Gregorian year number for dates close to 1 January. For example, 29 December 2014 is ISO 2015-W01-1, i.e., it is in year 2015 instead of 2014. A programming bug confusing these two year numbers is probably the cause of some Android users of Twitter being unable to log in around midnight of 29 December 2014 UTC.[2]

And here it is expressed in code using the PR:

import times

var dt1 = initDateTime(29, mDec, 2014, 0, 0, 0, 0, utc())
var week = dt1.getWeekOfYear()
var dt2 = initDateTime(dt1.weekday, week, dt1.year, 0, 0, 0, 0, utc())
echo dt1.year # Prints 2014
echo dt2.year # Prints 2013

This gotcha can be somewhat avoided by designing the API differently. I like Pythons isocalendar function which returns all components of a ISO week date (year, week, weekday) since it gives a hint to the caller that the ISO week date year is special. I would suggest doing something similiar for the times module. The difference between the two different year numberings should also be pointed out in the doc comment for the new overload of initDateTime, since the year it takes as input is different from the other overloads.

[timotheecour]

As mentioned in #17223 (review), iso weak is error prone and the API should be designed to prevent user errors.

let's instead introduce what python provide. I suggest the following API:

type IsoCalendar* = object
  year*: int
  weekIso*: range[1..53]
  weekDay*: WeekDay

proc isoCalendar*(): IsoCalendar = ...

proc initDateTime*(iso: IsoCalendar, hour: HourRange, minute: MinuteRange, second: SecondRange,
                   nanosecond: NanosecondRange, zone: Timezone = local()): DateTime {.since: (1, 5).} = ...

note

As discussed in https://stackoverflow.com/questions/2600775/how-to-get-week-number-in-python, the ISO week is not the most intuitive one, and there are other ones we may want to add in future, that are commonly used, eg:

https://docs.python.org/3/library/time.html

Week number of the year (Sunday as the first day of the week) as a decimal number [00,53]. All days in a new year preceding the first Sunday are considered to be in week 0.
Week number of the year (Monday as the first day of the week) as a decimal number [00,53]. All days in a new year preceding the first Monday are considered to be in week 0.

The API I suggested makes this possible without ambiguity, since we encapsulate with IsoCalendar, so other conventions can be added without confusion later, eg:

type XXXCalendar* = object ...
  # Week number of the year (Sunday as the first day of the week) as a decimal number [00,53]. All days in a new year preceding the first Sunday are considered to be in week 0.

note2

there should be a ..Warning explaining the gotcha's involved, along with runnableExamples that illustrate it (the current runnableExamples don't address this).

example: python

https://docs.python.org/3/library/datetime.html#strftime-and-strptime-behavior

>>> datetime.date(2021, 1, 1).isocalendar()
datetime.IsoCalendarDate(year=2020, week=53, weekday=5)
>>> datetime.date(2021, 1, 1).strftime("%W")
'00'
>>> datetime.date(2021, 1, 1).strftime("%V")
'53'
>>> datetime.date(2021, 1, 1).strftime("%U")
'00'

[capocasa]

@GULPF

Thanks for the feedback! Good catch!

I would prefer to solve it by having a getWeekYear() proc that will give you the iso year, and highlighted warning about the difference with examples as in @timotheecour's note 2.

Rationale:

1. Forcing the user to calculate 3 values when potentially only needing 1 is stylistically well suited to python, but not to Nim.
2. The datetime is designed as an object with procs that take primary types. Weekday is not a big enough feature to justify the inconsistency introduced by departing from that.

@timotheecour

note1: IsoCalendar is too object heavy a design for the Nim standard library, in my mind. It makes it is clumsy to use in a lot of cases: e.g.

• object with iso-num/day/year plus data about that date
• placing iso-num/day/year in local variables.

Using type IsoCalendar* = tuple would alleviate that by allowing unpacking.

Good point to think about extending the API to sunday weeknums. But I think having to type initDateTime(initIsoCalender(..)) wouldn't be worth it. I think it would be best to be add an enum parameter [iso/north-america] to the relevant procs and default to iso if sunday-based weeknums are introduced, because that's only clumsy for the sunday case, which is to my knowledge rare.

note 2: as mentioned above, I think this is a great idea too.

I'll gladly implement whatever consensus we end up with for the API design and added examples/docs.

[capocasa]

@timotheecour

type IsoCalendar* = tuple
  year*: int
  weekIso*: range[1..53]
  weekDay*: WeekDay

proc isoCalendar*(): IsoCalendar = ...

proc initDateTime*(iso: IsoCalendar, hour: HourRange, minute: MinuteRange, second: SecondRange,
                   nanosecond: NanosecondRange, zone: Timezone = local()): DateTime {.since: (1, 5).} = ...

[capocasa]

The more I think about it the more @timotheecour's suggestion to use an IsoCalendar makes sense to me, because it does sidestep a lot of potential confuse-the-weeknum-type bugs and it does make you think about what kind of year you are typing. Implementation works fine too.

I'm having a hard time getting over the parameter design of initDateTime(isocalendar, hour, minute, second, ns, timezone) though. Something about mixing an object type and several simple types just makes me shudder. In python that's not an issue because there is a date object.

[timotheecour]

I'm having a hard time getting over the parameter design of initDateTime(isocalendar, hour, minute, second, ns, timezone) though. Something about mixing an object type and several simple types just makes me shudder. In python that's not an issue because there is a date object.

how about:

type IsoCalendar* = object
  year*: int
  weekIso*: range[1..53]
  weekDay*: WeekDay

type DayTime* = object
  hour: HourRange, minute: MinuteRange, second: SecondRange, nanosecond: NanosecondRange

proc initDateTime*(isoDay: IsoCalendar, dayTime: DayTime, zone: Timezone = local()): DateTime

IMO, it makes sense to have a dedicated type for DayTime, it makes API's more composable

[capocasa]

TL;DR isocalendar is Pythonic, not Nimish. We should use a simple variant distinct type to help the user distinguish iso and regular years.

After taking a little time to think this through, I have come to the conclusion that we should do two things:

1. Release Iso week support with the current Api, adding a getYearWeek proc, and possible a variant distinct type for the iso year (non-iso week dates can be added later through an enum parameter)
2. Create a proper RFC to explore adding Date, DayTime, IsoCalendar (or IsoDate), and something like SundayWeekDate- or possibly not doing that and adding IsoDatetime and SundayDatetimeobjects.

Rationale:

My aesthetical reaction to the initDateTime(IsoCalendar, hour, ...) was symptomatic of the amount of changes that would be needed to be made in order to create a consistent API that also contains an IsoCalendar object (or tuple).

By adding an IsoCalendar object, you introduced a Date object. You must then, to keep the design good- by which I mean, consistent enough to be usable, because surprise-through-arbitrariness is avoided- also introduce a regular Date object, with a monthday, month and year. And if you do that, you are equally obliged to also implement a DayTime object.

But all of that only makes sense if you ware also willing to implement all the math for all the objects as related to each other and related to all the other times objects. If you introduce the objects without also implementing all the related procs, then you introduced dead objects. In a library, if I have objects, I need the math that goes with them in procs.

That is a lot of implementing just to give users a little nudge about the iso year, something they should know about anyway if they are going to use the concept. It's not that hard, you can see it in any calendar, physical or software, so the reminder in the docs should suffice- or potentially a simple variant distinct type for isoyear would solve the problem just as well, if not better, than python isocalendar does.

Now, of course, it might actually make sense to add all those objects and all those procs- that's what Python does, and that's why isocalendar feels elegant in Python. Personally, I missed Date and DayTime from Python (where it is called "Time") at first, but I quickly realized that I can just use a partially zeroed DateTime to represent dates or a TimeInterval shorter than 24 hours to represent time of day. So the minimalistic nature of the times module grew on me, and now I like it. But a PR about week dates is not really the best place to be discussing huge API changes for a module, so this should be explored in the context of a proper RFC that would explicitly accept or reject python-style date and daytime objects.

If don't add the Date and DayTime objects for regular dates, we can't add it for week day, either, without being infuriatingly arbitrary- something Nim has avoided so far, unlike, say, PHP (I know enough about PHP to want to avoid its design flaws at all cost, and arbitrary inconsistency tops the list). Therefore, I believe that the best choice is to follow as closely as possible the existing API design, which is what I did my best to do within the current pull request (minus the missing getWeekYear() proc that will be added).

Even this pull request were to get merged right now- and date, daytime and isocalendar were approved and added later- I think the resulting combined API is still good. Sometimes you need an iso week, and sometimes you need the full date, and you can get either.

So that is why I think we can afford to move forward with this pull request and add the missing functionality, and open the big can of worms later.

Comments and questions welcome! Thanks to everyone for contributing their thoughts so far.

[capocasa]

Done implementing changes!

• Added distinct IsoYear type to prevent week-based/regular year confusion, and allow adding initDateTime overloads for other week number calendars
• renamed getWeeksInYear to getWeeksInIsoYear to allow adding other week numbers
• removed getWeekYear proc
• Added getIsoWeekAndYear proc. iso-week calculation also yields the iso-year "for free", so it's most straight-forward to return both
• Examples now reflect differences between regular year and week-based ISO year
• Updated tests
• Updated changelog entry

Discussion:

The API design assumes no changes will be made to times.

If Date, IsoDate and DayTime objects were to be introduced, the current getIsoWeekAndYear would be easy enough to be replaced with, or called by, an additional getIsoDate proc.

I believe I have addressed all concerns raised in the discussion so far, although for practical reasons not necessarily in the suggested way. But I am really quite pleased by how much better this PR's quality got thanks to all your input. And I really appreciate the welcoming quick help addressing my newbie mistakes, as well.

If you feel you need to raise further issues, or have additional feedback, suggestions or questions, I'd be more than happy to address those, as well.

[ghost]

Sorry if my question is a bit rude, but is there any reason for this PR to be closed?

[capocasa]

Not rude at all! The reason is that I attempted to close the github app by pressing the pull request's "close" button, and I couldn't figure out how to reopen it until now, on Desktop.

[capocasa]

Rebased in the hope to successfully compete for scarce merge reviewing resources

[Araq]

Please add doc comments for these two new types.

[Araq]

As I said above, add a couple of more doc comments and then a changelog entry. Thanks!

[capocasa]

Thanks for the review! Changelog entry was already there from last time, added the mentioned docstrings.