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

Improve unit bubbling for ISO dates #2357

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@NobodysNightmare
Contributor

NobodysNightmare commented May 4, 2015

What is this?

This PR intends to fix issues with invalid ISO dates getting created from a well formatted moment.duration. GitHub Issues I am aware of:

What I have done is basically:

  • not use this.data as source of truth anymore for toISOString
    • I thought about changing the way data is filled, but I believe this would be the wrong approach, correct me if I am wrong
  • use some custom bubbling for toISOString that stops bubbling at hours and days
    • these are basically the borders that are also used internally
    • internal fields are milliseconds (which can bubble up to hours), days and month (becoming years)
  • add one-line spec that should cover all new edge cases

Why is it useful?

The ISO 8601 standard (which I only know from second hand knowledge), makes a difference between hours, days and months. That is:

  • P1D means tommorrow, at the same time
  • PT24H means in 24 hours, which can be different from the former case, if the clock changed in the meantime (think of DST changes, leap seconds, etc)
  • P30D means in thirty days, no matter how many days the current month has
  • P1M means on the same date in the next month

Thus from an ISO perspective there is a difference between moment.duration(1, 'days') and moment.duration(24, 'hours') which should now be reflected.

Note: I've written down similar arguments in the linked issues, but I believe it helps clarity to repeat it if I open a PR ^^

NobodysNightmare added some commits May 4, 2015

prevent bubbling of minor units for ISO dates
ISO 8601 makes a difference between e.g. 24 hours and 1 day:

* both are the same most of the time
* in case there is a clock change (e.g. DST) however, "P1D" means "next day, same time" and "PT24H" means: now in 24 hours (which can be off by one hour in either direction in case of DST)

days and months are also a complicated matter...
@NobodysNightmare

This comment has been minimized.

Contributor

NobodysNightmare commented May 5, 2015

Seems like the current develop branch is unstable. Travis reported two failures that I can reproduce locally on the develop branch alone.

I rebased my PR onto master, so that travis should report it green. Local results:

┌───────┬───────┬────────────┬────────┬────────┬─────────┐
│ Files │ Tests │ Assertions │ Failed │ Passed │ Runtime │
├───────┼───────┼────────────┼────────┼────────┼─────────┤
│ 1     │ 2377  │ 45713      │ 0      │ 45713  │ 6596    │
└───────┴───────┴────────────┴────────┴────────┴─────────┘

var h = abs(this.hours());
var m = abs(this.minutes());
var s = abs(this.seconds() + this.milliseconds() / 1000);
var Y = years;

This comment has been minimized.

@myabc

myabc May 5, 2015

@NobodysNightmare you should be able to use destructuring assignment here:

var [Y, M, D, h, m, s] = [year, months, days, hours, minutes, seconds];

This comment has been minimized.

@NobodysNightmare

NobodysNightmare May 6, 2015

Contributor

@myabc good idea, but:

We're using ES6 module system, but nothing else ES6

This comment has been minimized.

@myabc
@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 3, 2015

@NobodysNightmare thank you for working on this!

not use this.data as source of truth anymore for toISOString

You did the right thing. We store milliseconds and days separately exactly for this reason (days should go over DST as it doesn't exist, and hours, minutes, seconds should advance time exactly)

Seems like the current develop branch is unstable.

Sorry for that. Your change looks good. When I merge I make sure all tests pass (on top of the other changes scheduled for the release).

Will merge in next release.

@ichernev ichernev added this to the 2.10.5 milestone Jul 3, 2015

ichernev added a commit that referenced this pull request Jul 12, 2015

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 12, 2015

Merged in 6349dbf

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