icambron / twix.js

:hourglass::left_right_arrow: A date range plugin for moment.js
https://isaaccambron.com/twix.js/
MIT License
379 stars 54 forks source link

Correct way to get the start date #64

Closed jonagoldman closed 8 years ago

jonagoldman commented 9 years ago

I want to get the start date from a range. Wich is the correct way of doing it? I can iterate and get the first element but maybe theres a better option like range.first()? Can I use the 'start' property?

icambron commented 9 years ago

If you're not using all-day ranges, there's a start property, which you can definitely use (but not mutate). _trueStart handles all-day range, but isn't public, so you can use it, but I can't promise it won't change in future versions of Twix.

jonagoldman commented 9 years ago

I am using all-day ranges, so I should use _trueStart or iterate?

icambron commented 9 years ago

Go with iterate for now and I'll add trueStart() as an actual API soon.

jonagoldman commented 9 years ago

Thanks.

markstos commented 8 years ago

Getting the start time is valuable to use with the split feature. After calling split, it's useful to know the start and end time . Having them as moment objects would be ideal.

icambron commented 8 years ago

@markstos I'd be happy to take a PR that provides start() and end()

markstos commented 8 years ago

OK, I'll take a look.

markstos commented 8 years ago

There are already properties named start and end which conflict with adding a method of the same name. How would you like to resolve that conflict? It appears start and end may be only used internally, so they could renamed to _start, but having _start, _trueStart and start() will surely be a bit confusing.

It's also a bit confusing that there's no _trueEnd, but there is a _displayEnd, although no _displayStart. You said above that the current value for start() to return would be _trueStart, but should end() return end or _displayEnd?

icambron commented 8 years ago

Good questions. Yeah, I made everything hella confusing in my last update. The brother of trueStart is transferrableEnd. Both of those are terrible names. They only differ from start and end, respectively, in that they force the input times to the beginning of the day for all-day ranges. displayEnd is another all-day range thing.

I have an idea, but probably easier for me to just take a crack at it and see if I can clean things up.

icambron commented 8 years ago

@markstos I unmuddled the variable names a bit and turned start and end into accessor functions. Can you pull the develop branch give it a spin? (See 36759fe1b417deadd090bf1cabdffe975e3b41ba for the relevant change). This change will break some users who just call range.start as a property, but those dates dangerous enough to mutate that I think I'm OK with that.

markstos commented 8 years ago

@icambron, looks good to me. I contributed this recent PR to improve test coverage and docs for start() and end()

https://github.com/icambron/twix.js/pull/78

In my first pass at writing the test coverage, the tests failed, because I had copied a nearby test and ignored the word 'true'. I later realized this was a shorthand: {allDay:true}, but the boolean shortcut is definitely less clear for people reading related code who aren't already familiar with twix.

Thanks for the first response! start() and end() will clean up our code.

We are using it to integrate the RideAmigos platform with the Moves app-- commuters will be able use Moves to automatically log trips when they walk or bike to work, and then later be rewarded in incentives programs for cleaner commutes.

We can only pull down so much data through their API at once, so we use split up data ranges into smaller chunks using Twix.

icambron commented 8 years ago

Cool, I'll cut a release soon.

the boolean shortcut is definitely less clear for people reading related code who aren't already familiar with twix

It's less a shortcut than it is legacy, but I definitely hear you. I've tried to deemphasize it in the docs, but it's everywhere in the tests. One day I'll go through and convert them all.

so we use split up data ranges into smaller chunks using Twix.

Neat. Always good to hear how people are using Twix. Good luck with your project!