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

duration can be in inconsistent state after subtraction #2166

Closed
zacronos opened this Issue Jan 16, 2015 · 6 comments

Comments

Projects
None yet
6 participants
@zacronos

zacronos commented Jan 16, 2015

I haven't explored to see all the circumstances that can cause it, but I notice that a duration's fields do not necessarily get normalized after subtraction. Depending on how you do the same subtraction (conceptually), you get different results.

Here's a fiddle to see the issue in action:
http://jsfiddle.net/9cs8o1v1/3/

A test that would break due to this bug is:

var subtract1 = moment.duration(600, "days").subtract(1, "year");
var subtract2 = moment.duration(600, "days").subtract(365, "days");
assertEqual(subtract1.months(), subtract2.months());

As you can see in the fiddle, subtract1.months() yields -5, while subtract2.months() yields 7.

@zacronos

This comment has been minimized.

zacronos commented Jan 17, 2015

The problem seems to be more extensive than shown in that first fiddle.

http://jsfiddle.net/9cs8o1v1/4/
The result after subtraction, when the duration is handled as milliseconds, is the same in every case, so at some level things are consistent and correct. But retrieving the fields individually, and looking at the humanized result, shows that only subtraction as days seems to be fully correct at least in this example.

http://jsfiddle.net/9cs8o1v1/5/
In this one, even the duration as milliseconds is sometimes not correct, even though the individual fields wind up correct in each case. The humanized result is also wrong too, but the cases where the humanized result is wrong and the 'as milliseconds' result is wrong don't entirely match up.

@zacronos zacronos changed the title from duration should normalize field values after subtraction to duration can be in inconsistent state after subtraction Jan 17, 2015

@zacronos

This comment has been minimized.

zacronos commented Jan 17, 2015

Now I think this issue runs deeper than duration.subtract(), so maybe it duplicates an issue covered elsewhere -- I only searched for tickets regarding subtract before creating this one. Without spending more time exploring, I'm not sure how far the inconsistencies go.

var dur1 = moment.duration({months: 2});
assertEqual(dur1.months(), 2);
assertEqual(dur1.days(), 0);

// conceptually, this should yield the same duration as dur1
var dur2 = moment.duration(dur1.asDays(), "days");
assertEqual(dur2.months(), dur1.months());
assertEqual(dur2.days(), dur1.days());   // FAILS

The final assert fails, as dur2.days() is 1 instead of 0.

@clarkorz

This comment has been minimized.

clarkorz commented Jan 23, 2015

I meet the same problem. It seems that moment only bubble up values (e.g.: PT48H becomes P2D), but it never bubble down values. check this line

var d = moment.duration('P1D').subtract(1, 'hour');
d.toString(); // P1DT1H
d.days(); // 1
d.hours(); // -1
d.asHours(); // 23

As you can see, the as* methods works fine. So the workaround is always call asMilliseconds() after subtraction:

var d = moment.duration(moment.duration('P1D').subtract(1, 'hour').asMilliseconds());
d.toString();// PT23H
d.hours(); // 23
@mesk41in

This comment has been minimized.

mesk41in commented Feb 20, 2015

I think this is the same issue (version : 2.9.0):

var a = moment('2015/01/01'),
    b = moment('2015/01/31');

console.log(b.diff(a, 'days')); // 30 - correct
console.log(moment.duration({from: a, to: b}).as('days')); // 30 - correct

b = moment('2015/02/01');

console.log(b.diff(a, 'days')); // 31 - correct
console.log(moment.duration({from: a, to: b}).as('days')); // 30 - incorrect
@StefanCodes

This comment has been minimized.

StefanCodes commented May 28, 2015

I've run across this, too. Not much to add, but this can be demonstrated with both .add() (using a negative value) and .subtract():

moment.duration(1, 'days').add(moment.duration(-1, 'hours')).hours()
> -1

moment.duration(1, 'days').subtract(moment.duration(1, 'hours')).hours()
> -1

You can coax the desired value by constructing a new duration from the result of asMilliseconds():

moment.duration(moment.duration(1, 'days').subtract(moment.duration(1, 'hours')).asMilliseconds()).hours()
> 23

@mj1856 mj1856 added the Bug label Jun 19, 2015

@ichernev

This comment has been minimized.

Contributor

ichernev commented Jul 8, 2015

I agree we have a problem here.

The main issue is that

  • we keep durations as 3 fields (ms, days and months)
  • this means you might end up with negative values in one field, and positive in another
  • bubbling doesn't handle a mix of positive / negative values (only all positive and all negative)

One way to fix this is to add code to bubble so it works well with a mix of positive/negative values. If it detects positive/negative, it will just convert everything to ms, and then bubble that.

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