add failing test for all-day endpoint

[benesch]

@icambron, exactly as you suspected, including midnight of the next day in all day ranges is breaking some of our code. Since you mentioned that the change makes Twix more consistent and its code simpler, this PR only adds a failing test.

But basically, it seems like the following shouldn't be true:

moment.twix("2013-04-12", "2013-04-12", true).contains("2013-04-13")

Let me know if I can help with this at all!

[icambron]

Right, so that's the core behavior change I just made, i.e that's expected behavior now. But it's fair to ask whether it should be or not. My thinking is that it should be consistent with non-all-day ranges.

start.twix(end).contains(end); //=> true

Which is how it's always been. If an all day range can be said to "end at midnight" then midnight should be included in the range the same way as it would be for a regular range includes its end. That consistency also simplifies the implementation, which over time had sprouted special logic to keep the interpretation of "end" separate in all-day and non-all-day ranges.

That said, I'm receptive to arguments along the lines of "that's great in theory, but in practice it makes all-day ranges kind of a pain in these use cases".

[icambron]

@benesch Closing this for lack of activity. Happy to reopen it, although now that it's been in the wild for a while, I'm loathe to break it again.

[benesch]

Sounds good to me. I'm no longer involved with the project for which this behavior was problematic, so whatever makes the code simpler!

@butterflyhug—bringing this to your attention so it doesn't bite you if/when you upgrade Twix.

[butterflyhug]

@benesch Thanks for the heads up.

This will probably prevent us from upgrading to any new Twix releases for some time to come, but I can't bring myself to strenuously disagree with making all-day ranges behave consistently with everything else.

Really, it would be ideal for us if all our Twix ranges were half open (like the old all-day ranges) -- we've ended up faking that a fair amount in our code. If you're receptive to including that complexity in the library, I may be willing to put together a PR to implement open and closed bounds as options for one or both endpoints of any arbitrary Twix range... but even then, it'll likely be a few weeks before I get enough spare bandwidth to tackle that.

[icambron]

@butterflyhug - Thanks. I'm not against adding half-open range support, as long as it's a) reasonably simple implementation-wise and b) is implemented for all the methods. Newer versions of Twix allow the third argument to be an option object instead of a boolean (e.g. {allDay: true}), so you could add startOpen and endOpen or somesuch.

[icambron]

@benesch @butterflyhug This is a couple months too late in terms of communication, but while I'm thinking about it: I reconsidered this issue a while ago when I realized how much time I was spending on the confusion the behavior discussed here created. Currently:

> moment.twix("2013-04-12", "2013-04-12", true).contains("2013-04-13")
false
> moment.twix("2013-04-12", "2013-04-13").contains("2013-04-13")
true

IOW, the currently released behavior is all-day ranges are half-open, non-all-day are closed, like it was before I made the change that spawned this ticket. I find the split conceptually awkward, but the everything-is-closed behavior was so unintuitive that even I kept forgetting about it (e.g. here). I did some magic to still format strings as ending at midnight, even though it secretly ends on the last MS, which deals with the implementation concerns I had with the old-old-way.

I don't know how exactly this effects the implementation of #66 or whether it means you don't really need/want it now.

[butterflyhug]

Yep, awesome! Thanks for closing the loop; half-open all-day ranges mean we'll be able to upgrade to current Twix (and Moment 2.10.x) even though I haven't had the free time to finish #66.