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

change add subtract to handle decimal values by rounding #2970

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
4 participants
@maggiepint
Member

maggiepint commented Feb 18, 2016

Normalizes behavior of add and subtract when a decimal value is passed.
Current behavior:

moment('2016-04-03').add(1.5, 'days').format() = "2016-04-04T00:00:00-05:00"  // (adds 1)
moment('2016-04-03').add(-1.5, 'days').format() = "2016-04-01T00:00:00-05:00"  // (adds -2)
moment('2016-04-03').subtract(1.5, 'days').format() = "2016-04-01T00:00:00-05:00"  // (subtracts 2)
moment('2016-04-03').subtract(-1.5, 'days').format() = "2016-04-04T00:00:00-05:00"  // (subtracts -1)

Behavior after PR:

moment('2016-04-03').add(1.5, 'days').format() = "2016-04-04T00:00:00-05:00"  // (adds 1)
moment('2016-04-03').add(-1.5, 'days').format() = "2016-04-02T00:00:00-05:00"  // (adds -1)
moment('2016-04-03').subtract(1.5, 'days').format() = "2016-04-02T00:00:00-05:00"  // (subtracts 1)
moment('2016-04-03').subtract(-1.5, 'days').format() = "2016-04-04T00:00:00-05:00"  // (subtracts -1)

The same behavior happens with months.

I understand where this could be interpreted as a breaking change, so I certainly won't be offended if you don't take it, but I think it definitely causes the API to behave in a manner closer to what the user would expect.

@maggiepint

This comment has been minimized.

Member

maggiepint commented Feb 21, 2016

For reference, this would close #2763 and #2964

@icambron

This comment has been minimized.

Member

icambron commented Feb 22, 2016

Yup, +1

@mj1856 mj1856 added this to the 2.12.0 milestone Feb 24, 2016

@mj1856

This comment has been minimized.

Member

mj1856 commented Feb 24, 2016

Looks good to me. Can you also verify it covers the edge cases I showed in #2964? Thanks.

@maggiepint

This comment has been minimized.

Member

maggiepint commented Feb 26, 2016

@mj1856 - note the tests I added related to quarter and year. I think this is what we want, even though it's not consistent with day and month, but it's your call. If we want those to truncate too, I can certainly do that. I'm just fairly confident that it will break someone's code.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 2, 2016

I think the change makes sense, but I'd prefer if we rounded double values, rather than absFloor them as we do now. It might happen that some computation produces 2.99 instead of 3, so that will cause some unexpected results.

@maggiepint

This comment has been minimized.

Member

maggiepint commented Mar 2, 2016

Fine by me. I'll throw it in tonight.

@maggiepint maggiepint changed the title from change add subtract to handle decimal values by truncation to change add subtract to handle decimal values by rounding Mar 3, 2016

@maggiepint

This comment has been minimized.

Member

maggiepint commented Mar 3, 2016

@ichernev I kind of hate this as Math.round() because it results in

moment().add(-1.5, 'days')

and

moment().subtract(1.5, 'days')

having different results.

I think if we don't want absFloor then we want absRound. That way adding the negative and subtracting the positive are always the same.
That okay with you?

@maggiepint

This comment has been minimized.

Member

maggiepint commented Mar 5, 2016

Now this is absRound. I think I'm happy with it.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 5, 2016

Just make sure to round to infinity or zero, in case of .5. The whole fix
is to make positive and negative act the same.
On Mar 4, 2016 8:14 PM, "maggiepint" notifications@github.com wrote:

Now this is absRound. I think I'm happy with it.


Reply to this email directly or view it on GitHub
#2970 (comment).

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 6, 2016

I'll fix a few mistakes in the test comments because of the floor->round change.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 6, 2016

Merged in d46af5c

@ichernev ichernev closed this Mar 6, 2016

ichernev added a commit that referenced this pull request Mar 6, 2016

Merge pull request #2970 from maggiepint:decimalAddSubtract
change add subtract to handle decimal values by rounding

@maggiepint maggiepint deleted the maggiepint:decimalAddSubtract branch Mar 7, 2016

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