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

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

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.

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.

@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 :)

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

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

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

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

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.

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));

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.

I agree!

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

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

Merged in 670d6c2