Day difference DST issue

[arttsu]

An attempt at fixing https://github.com/rluiten/elm-date-extra/issues/21

[rluiten]

Another day light saving complication fun stuff...

Here are some thoughts around this issue I had last night.

The Period model is intended to represent an elapsed time concept with no compensation for interesting human abstractions like Years with variable number of days, Months with variable number of days and Day light saving back flips. So I don't like the idea of modifying it for this behavior. (I think I may need to clarify this better in the Period module header doc comment)

The Duration model is suited to this and it makes sense for it to compensate for day light saving stuff in there but it has extra baggage with Years, Months etc.

In addition any change to diff behavior also indicates that add behavior needs to change in an equivalent manner. So adding a diff to a date produces the other date again.

Given changes to both diff and add in Duration covers part of the requirement. Then need a way to get the number of day in a Duration relative to a reference Date. (require reference date because Duration needs to know about Year/Month/Date to compensate for extra days hours etc.).

In Duration module add something like asDays : Period -> Date -> Int

Then look at using Duration.diff and Duration.asDays in isoWeek very similar to how you did things.

Just realized Duration already has day light saving compensation code in its add logic. But the diff function does not similarly compensate, that is a bug I am pretty sure :).

Side Notes that have come up while exploring issues.

I am doubting the value of the Period.Week and Duration.Week in the models. They are of limited value and easy to for anyone to get themselves from Day if they need it.

[arttsu]

Thank you for the detailed answer. I'd like to discuss a few more points.

But the diff function does not similarly compensate, that is a bug I am pretty sure

I'm not sure if it needs to. As far as I understand the code, add needs to compensate, because it uses Period.add internally, but diff just compares calendar values. Also, it seems to work as described in documentation in the repl:

> mar28 = Create.dateFromFields 2016 Date.Mar 28 12 0 0 0
<Mon Mar 28 2016 12:00:00 GMT+0300 (EEST)> : Date.Date
> mar21 = Create.dateFromFields 2016 Date.Mar 21 12 0 0 0
<Mon Mar 21 2016 12:00:00 GMT+0200 (EET)> : Date.Date
> delta = Duration.diff mar28 mar21
{ year = 0, month = 0, day = 7, hour = 0, minute = 0, second = 0, millisecond = 0 }
    : Date.Extra.Duration.DeltaRecord
> Duration.add (Duration.Delta delta) 1 mar21
<Mon Mar 28 2016 12:00:00 GMT+0300 (EEST)> : Date.Date
> Duration.add Duration.Day 7 mar21
<Mon Mar 28 2016 12:00:00 GMT+0300 (EEST)> : Date.Date

In Duration module add something like asDays : Period -> Date -> Int

Then look at using Duration.diff and Duration.asDays in isoWeek very similar to how you did things.

Have you meant using Period.diff and Duration.asDays in isoWeek? I haven't been able to think of how to extract number of days from Duration.DeltaRecord, in a way that doesn't feel like doing the same work twice. Maybe I'm missing something obvious though :-)

The Period model is intended to represent an elapsed time concept with no compensation for interesting human abstractions like Years with variable number of days, Months with variable number of days and Day light saving back flips.

Period.diff still isn't working quite right, I think.

> sep12 = Create.dateFromFields 2016 Date.Sep 12 0 0 0 0
<Mon Sep 12 2016 00:00:00 GMT+0300 (EEST)> : Date.Date
> jan4 = Create.dateFromFields 2016 Date.Jan 4 0 0 0 0
<Mon Jan 04 2016 00:00:00 GMT+0200 (EET)> : Date.Date
> Period.diff sep12 jan4
{ week = 35, day = 6, hour = 0, minute = 0, second = 0, millisecond = 0 }
    : Date.Extra.Period.DeltaRecord

This adds up to just 251 days. I'd expect it to return { week = 35, day = 6, hour = 23, ... }

Moment.js:

moment("2016-09-12").diff(moment("2016-01-04"), "days")
252

Maybe it'd be better to just use round in isoWeek, document it as a temporary solution and don't change anything else for this particular fix? Are there any other use cases at the moment, where similar logic would be useful?

[rluiten]

I havn't forgotten this, been busy, and I want to make sure i allocate a bit of time once I get into it.

I'm not sure if it needs to. As far as I understand the code, add needs to compensate, because it uses Period.add internally, but diff just compares calendar values. Also, it seems to work as described in documentation in the repl:

You are right, I was over thinking it. Though I do think there might be a bug in the way it currently does daylight compensation as it only tries to compensate for day light saving for Days, Week,s Months and years which may be wrong I believe, since adding 1 millisecond can cross a day light saving boundary.

I haven't been able to think of how to extract number of days from Duration.DeltaRecord, in a way that doesn't feel like doing the same work twice.

Yes its a lot of work to do it, as much work as calculating the diff to start with. I will have to explore things a bit further when I allocate time to work on this.

I do wish it was easier to run tests in different time zones, at the moment I have a notebook here that I can remote to and change its timezone to test things manually :). My home machine time zone has no daylight saving and I really don't want muck with its time zone as I have a lot of stuff on it.

Cheers Rob.

[rluiten]

Closing this as have fixed.