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

Added inclusivity parameter to isBetween method. #2991

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
4 participants
@dbkirk4211
Contributor

dbkirk4211 commented Feb 27, 2016

I didn't want this to go stale so I rewrote the necessary files.

The user can determine inclusivity via a fourth parameter by passing '()", '[)', '(]', or '[]'. The default behavior is '()'.

@mj1856

This comment has been minimized.

Member

mj1856 commented Feb 28, 2016

Please rebase to squash/fixup into a single commit. Thanks.

@mj1856 mj1856 added the Enhancement label Feb 28, 2016

@dbkirk4211

This comment has been minimized.

Contributor

dbkirk4211 commented Feb 29, 2016

I'll try to get at it tonight. Thank you.

@maggiepint

This comment has been minimized.

Member

maggiepint commented Mar 2, 2016

This is a minor concern, but in the entire Moment.js code base there is only one instance of

throw new Error();

That one instance is in a place where there is no possible way to sensibly default. Here though, one could fall back to the current behavior if the input is incorrect.
It seems to me like doing this would be more in line with the current patterns in use in the code base.

Personally, I actually like the throw - descriptive error messages are awesome - but it feels like it's a departure from the original developer intent.

@dbkirk4211 dbkirk4211 closed this Mar 3, 2016

@dbkirk4211 dbkirk4211 reopened this Mar 3, 2016

@dbkirk4211

This comment has been minimized.

Contributor

dbkirk4211 commented Mar 3, 2016

As it is written, the method defaults to "()" inclusivity if the user passes nothing, null or false.

If the user passes "{]" instead of "(]" or "[}" instead of "[)", etc.; he or she would receive the error. This way, an error message is presented to the user instead of the default behavior of "()". This is useful when a method is expecting a finite amount of possibilities.

This is something that could be done in the normailzeUnits method.

Sorry it took so long to squash the commits, I have a deadline tomorrow I was working towards.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 3, 2016

@dbkirk4211 in practice nobody knows how the method is to be used, where the input is coming, and what the calling code expects. Also different libraries have different philosophies and deal with one problem in different ways.

There are more severe cases where we ignore errors silently, because stopping the execution of an important piece of code can render the entire web site unusable, whereas a single broken date can only do less damage :)

@dbkirk4211

This comment has been minimized.

Contributor

dbkirk4211 commented Mar 3, 2016

I agree. Too easy. I'll edit the PR's commit later today.

@dbkirk4211

This comment has been minimized.

Contributor

dbkirk4211 commented Mar 4, 2016

Even more compact and down to the nitty gritty! Have a good night.

@dbkirk4211

This comment has been minimized.

Contributor

dbkirk4211 commented Mar 5, 2016

@maggiepint @mj1856 @ichernev Do y'all see anything else I could edit or add?

@maggiepint

This comment has been minimized.

Member

maggiepint commented Mar 5, 2016

LGTM but ultimately what goes into production is up to @ichernev

@maggiepint

This comment has been minimized.

Member

maggiepint commented Mar 19, 2016

I want to move this feature along, so I'm tagging it for next release. Code looks fine, and I personally prefer this API to the options object one.

This does not stop this pull request, but I want to point out the following code:

var m = moment(new Date(2011, 3, 2, 3, 4, 5, 10));
assert.equal(m.isBetween(
         moment(new Date(2011, 3, 2, 3, 4, 5, 10)),
         moment(new Date(2011, 3, 2, 3, 4, 5, 10)), 'milliseconds', '[)'), false, 'start is included and end is excluded, should fail on same end and start date');

Obviously programmatically this makes perfect sense - two conditions are evaluated and one is false, ergo the whole thing is false.

If you aren't thinking about the code in this way though, I feel like it could take someone by surprise. Why should it be false? Why not true? I feel like both are sort of equally correct, so we need to document that it does this.

@maggiepint maggiepint added this to the 2.13.0 milestone Mar 19, 2016

@dbkirk4211

This comment has been minimized.

Contributor

dbkirk4211 commented Mar 20, 2016

I can see it both ways. I guess the question is whether moment will allow the "from" and "to" parameters to be the same. If the answer is yes, then it should be changed. If not, then both "[)" and "(]" will be affected, if "from" and "to" are equal.

Possible fix:
"[)"
return (this.isSame(from) || (this.isAfter(from, units) && this.isBefore(to, units));

"(]"
return (this.isSame(to) || (this.isAfter(from, units) && this.isBefore(to, units));

@maggiepint

This comment has been minimized.

Member

maggiepint commented Mar 20, 2016

moment('2016-01-01').isBetween(moment('2016-01-01'),moment('2016-01-01'))
false

As you can see, currently if the from and the to are the same, the result is false (which makes sense because it excludes the ends by default.

I'm inclined to just leave the code as-is, but document the behavior. This keeps everything basically the same.

@dbkirk4211

This comment has been minimized.

Contributor

dbkirk4211 commented Mar 21, 2016

I agree!

@@ -29,7 +29,14 @@ export function isBefore (input, units) {
}
}
export function isBetween (from, to, units) {
export function isBetween (from, to, units, inclusivity) {
if (inclusivity === '(]') {

This comment has been minimized.

@ichernev

ichernev Apr 12, 2016

Contributor

Can't we do something like:

inclusivity = inclusivity || '()';
return (inclusivity[0] === '(' ? this.isAfter(from, units) : !this.isBefore(from, units)) && (inclusivity[1] === ')' ? this.isBefore(to, units) : !this.isAfter(from, units));

This comment has been minimized.

@dbkirk4211

dbkirk4211 Apr 12, 2016

Contributor

I love using ternary operators. The tradeoff is readability, but I don't think that is a problem here. Other than the mistype at the end, it looks solid. inclusivity = inclusivity || '()' takes care of inclusivity ever being undefined and it has a smaller footprint. I wrote the commit to be representative of the current code base; but like I said, I have a soft spot for ternary operators.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 12, 2016

Thanks @dbkirk4211 , I added a small implementation note you can address.

@dbkirk4211

This comment has been minimized.

Contributor

dbkirk4211 commented Apr 13, 2016

@ichernev I looked it over again and I still concur with your snippet. Ultimately, it's a decision on style and bytes.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Apr 16, 2016

Merged in 670d6c2

@ichernev ichernev closed this Apr 16, 2016

ichernev added a commit that referenced this pull request Apr 16, 2016

ichernev added a commit that referenced this pull request Apr 16, 2016

Merge pull request #2991 from dbkirk4211:develop
Added inclusivity parameter to isBetween method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment