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

Add relative option parameter to businessDiff #54

Merged
merged 1 commit into from
Apr 17, 2019

Conversation

lyleunderwood
Copy link
Contributor

Fixes #32.

This changes behavior of businessDiff, and may break existing code which relies on current behavior. As this PR stands and according to semver, this may constitute a major version bump. Maybe this could instead be implemented under an alternate function name or as an option to businessDiff until a major version comes around?

The goal here is to bring the behavior of businessDiff in-line with the behavior of moment's diff, as specified here:

If the moment is earlier than the moment you are passing to moment.fn.diff, the return value will be negative.

@mcdado
Copy link
Collaborator

mcdado commented Dec 11, 2018

Good idea about using an option parameter. I guess the default should be to keep the current behaviour, while the next major bump it could be switched. Could you implement it in your PR please?

@lyleunderwood
Copy link
Contributor Author

@mcdado added a relative: boolean param

Copy link
Contributor

@oleg-koval oleg-koval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lyleunderwood
Copy link
Contributor Author

Realized i had mistakenly removed a test that was still relevant, also squashed.

@mcdado
Copy link
Collaborator

mcdado commented Apr 10, 2019

As spoken in #32 this can be merged. LGTM.

@lyleunderwood one thing though… in the README example you omitted the second parameter true, shouldn't it have it?

@lyleunderwood
Copy link
Contributor Author

@mcdado nice catch, fixed

@lyleunderwood lyleunderwood changed the title make businessDiff return negative values Add relative option parameter to businessDiff Apr 17, 2019
@mcdado mcdado merged commit 2706a3e into eduolalo:master Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

businessDiff only returning absolute value
3 participants