Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

abstract-over DST change in .diff() #94

Closed
hunteva opened this issue Feb 5, 2019 · 4 comments
Closed

abstract-over DST change in .diff() #94

hunteva opened this issue Feb 5, 2019 · 4 comments

Comments

@hunteva
Copy link
Contributor

hunteva commented Feb 5, 2019

Sympton

> from = spacetime("2019-02-05 00:00:00")
SpaceTime { epoch: 1549278000878, tz: 'pacific/auckland', silent: true }
> from = spacetime("2019-02-05 00:00:00.0")
SpaceTime { epoch: 1549278000000, tz: 'pacific/auckland', silent: true }
> to = from.clone().minus(20, 'years')
SpaceTime { epoch: 918126000000, tz: 'pacific/auckland', silent: true }
> to.diff(from)
{ milliseconds: 631152000000,
  seconds: 631152000,
  minutes: 10519200,
  hours: 175320,
  years: 20,
  months: 240,
  weeks: 1047,
  days: 7305 }

Weeks should be 1043 instead of 1047

Cause

Week diff use climb way to adds weeks one by one until it reaches the target, the underlying adds function collects ms,sec,hour before the addition and then feeds them to the 'walk(s, want)' function to walk around and get precise match, but it could potentially step back for 23 hours rather than 1 hour while cross from non-dst to dst dates as shown in above, which cause the outer diff loop went more round and produce more week difference.

Root cause

ok, just found the root of this:

> a = spacetime('2018-03-29 00:00:0.0')
SpaceTime { epoch: 1522234800000, tz: 'pacific/auckland', silent: true }
> b = a.clone().add(1, 'week')
SpaceTime { epoch: 1522756800000, tz: 'pacific/auckland', silent: true }
> b.since(a)
{ diff:
   { years: 0, months: 0, days: 6, hours: 24, minutes: 0, seconds: 0 },
  rounded: '7 days ago',
  qualified: 'almost 7 days ago',
  precise: '6 days, 24 hours ago' }
> a.format('{mdy} {time-24}')
'03/29/2018 0:00'
> b.format('{mdy} {time-24}')
'04/04/2018 0:00'

it should be '04/04/2018 23:00' because dst instead of '0:00', the 'add' function actually step back for 23 hour.

Fix

  • A 'dst compensate' step before it went for a 'walk'.
    - or just normal ms division instead of climb for 'week' diff

please @spencermountain, would like to hear your advise

definite need to fix the add & walk issue, should be come up a fix shortly

@spencermountain
Copy link
Owner

spencermountain commented Feb 5, 2019

hey, thanks for your help.
Yeah, in my opinion, .add(1, 'week') should compensate for the dst change, and move a full 7 days. That seems to be working right now.

it looks like the confusion is in the .diff() results then.
We may want to add this 'dst compensate' stuff to .diff.

hmmm. Do you agree?

(ps you don't need to use .clone() anymore for that)

@spencermountain spencermountain changed the title wrong 'week' diff calculation for distant dates due to DST abstract-over DST change in .diff() Feb 5, 2019
@hunteva
Copy link
Contributor Author

hunteva commented Feb 6, 2019

Hi, @spencermountain, thanks for merge my changes, I'm really happy to help here.

Just want to be clear that current implementation is what your described, it works like this

> spacetime('2018-03-29T00:00:00.000+13:00').add(1, 'week').format('iso')
2018-04-04T23:00:00.000+12:00

Not exactly 'full 7 days' literally but 7 days worth of ms.

I'm totally agree the idea of dst awareness diff, just haven't figure out what's the more proper way to interprete the results, eg:

start = '2018-03-29T00:00:00.000+13:00'

  • Options 1, loyally repensents the ms
    diff('2018-04-04T23:00:00.000+12:00', start) output 7 days
    diff('2018-04-05T00:00:00.000+12:00', start) output 7 days 1 hour

  • Options 2, compensate to dst, but might be weird looking
    diff('2018-04-04T23:00:00.000+12:00', start) outputs 6 days 24 hours (because dst crossing on 01/Apr makes that day 25 hours long. it might end up been xxxxx days xxx hours for long distant dates crossing dst multiple times)
    diff('2018-04-05T00:00:00.000+12:00', start) outputs 7 days

any suggestions?

@spencermountain
Copy link
Owner

oh hey, you're completely right. Thank you. I was wrong yesterday.
yeah, adding a week to thursday at 4pm should always return a result at thursday at 4pm.

the add function is not correct then. The diff calculation can just use the same methods, so will not require any additional dst awareness step.

Sorry about this. I can take a look at this, this week - or I will happily merge a PR with however you decide to fix it.
thanks!

@hunteva
Copy link
Contributor Author

hunteva commented Feb 7, 2019

hi, @spencermountain the PR has been filed, now add function does exactly what we discussed for 'year/season/quarter/month/week/days', but not for 'hour/minutes/sec' since that's some exact number user ask to add.

Please let me know your thoughts, Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants