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

Decimals in add days and months don't work when negative #2964

Closed
maggiepint opened this issue Feb 16, 2016 · 3 comments
Closed

Decimals in add days and months don't work when negative #2964

maggiepint opened this issue Feb 16, 2016 · 3 comments
Labels

Comments

@maggiepint
Copy link
Member

Kind of an obscure use case, but of the following tests, the second and fourth fail:

assert.equal(moment([2016,3,3]).add(1.5, 'days').date(), 4, 'adding 1.5 days adds 1 day');
assert.equal(moment([2016,3,3]).add(-1.5, 'days').date(),2, 'adding -1.5 days subtracts 1 day');
assert.equal(moment([2016,3,3]).add(1.5, 'months').month(), 4, 'adding 1.5 months adds 1 month');
assert.equal(moment([2016,3,3]).add(-1.5, 'months').month(), 2, 'adding -1.5 months subtracts 1 month');
moment([2016,3,3]).add(-1.5, 'days').date() = 1
moment([2016,3,3]).add(-1.5, 'months').month() = 1

As you can see, adding 1.5 of a non-deterministic unit ends up adding 1 of it, while adding -1.5 ends up subtracting 2 of it.

I feel as though this behavior should be consistent (1 both ways).

Month and day should be coerced to integer values from floats. They aren't right now, and that's what is causing this. #2430 would make it clear that this is what is intended. Since all other time frames (year, quarter, week) are converted to months and days, only months and days would need to be coerced.

I have this fixed locally, but I want to add a whole bunch of unit tests because there was no unit test coverage of adding or subtracting floats, or of adding and subtracting negatives, so I want to cover everything.

Let me know if this isn't the desired behavior. An alternative would be to simply invalidate the date if a float was passed. I wouldn't mind this.

@icambron
Copy link
Member

I think coercing them is the most consistent thing to do.

@mattjohnsonpint
Copy link
Contributor

Had to think about this one a bit, but yes, I do think it's a bug. At least, it is when you consider the entire state of the result. One might think 1 was the correct answer, since subtracting 1.5 days from midnight on the 3rd should leave you at noon on the 1st. However, as we've discussed before, we don't subdivide days, months, or years in the add/subtract methods. When you look at the full result:

moment([2016,3,3]).add(-1.5, 'days').format()  // "2016-04-01T00:00:00-07:00"

So it's not using the half-day at all, which is fine. But then we really are only subtracting one day, so yes - it should result in the 2nd, not the 1st.

Consider also that the same thing for adding negative numbers with the add function also happens with subtracting positive numbers with the subtract function. Both will need to be fixed.

Some more interesting observations:

moment([2016,1,3]).subtract(1, 'days').format()    // "2016-02-02T00:00:00-08:00"
moment([2016,1,3]).subtract(1.1, 'days').format()  // "2016-02-01T00:00:00-08:00"
moment([2016,1,2]).subtract(1.1, 'days').format()  // "2016-01-31T00:00:00-08:00"
moment([2016,1,1]).subtract(1.1, 'days').format()  // "2016-01-31T00:00:00-08:00"

The last one is interesting. When it wraps around to the prior month, then it is correct. But otherwise it's off by a day. 😕

@mattjohnsonpint
Copy link
Contributor

We'll track in PR #2970. Thanks.

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

No branches or pull requests

3 participants