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

Added toArray() #94

Closed IvanJov closed 7 years ago

IvanJov commented 7 years ago

Hi! I was using Twix on previous project and I think that's pretty useful to have twix.toArray() helper function.

If you like it I am happy to write some text about it in documentation. Usage is very simple, it has 4 arguments, first one is format of the moment object that will be in array, other 3 are same as iterate function.

Example: moment.utc('1982-05-25').twix(moment.utc('1982-05-27'), allDay: true).toArray('YYYY-MM-DD', 'days') Output: ['1982-05-25', '1982-05-26', '1982-05-27']

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.04%) to 97.927% when pulling 2da0ad96d9e082772c0ddbea7c3b725512d32389 on IvanJov:master into a03ff817986a0dc4276c73d8861f7117083e9450 on icambron:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.04%) to 97.927% when pulling 2da0ad96d9e082772c0ddbea7c3b725512d32389 on IvanJov:master into a03ff817986a0dc4276c73d8861f7117083e9450 on icambron:master.

icambron commented 7 years ago

I like the toArray() idea but it shouldn't be responsible for the formatting; it should just return times. You can always do this:

t.toArray().map((m) -> m.format("YYYY-MM-DD"))

Or perhaps it can have a second optional mapper argument like this:

t.toArray((m) -> m.format("YYYY-MM-DD"))
IvanJov commented 7 years ago

@icambron You are right, I will remove the formatting! And I don't like the idea with mapperargument, maybe that's too much also :)

What do you think?

IvanJov commented 7 years ago

Let me know if this looks better!

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.04%) to 97.927% when pulling fae62a81b1bed8462a53b6e073d9fabdad2e8827 on IvanJov:master into a03ff817986a0dc4276c73d8861f7117083e9450 on icambron:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.04%) to 97.927% when pulling fae62a81b1bed8462a53b6e073d9fabdad2e8827 on IvanJov:master into a03ff817986a0dc4276c73d8861f7117083e9450 on icambron:master.

icambron commented 7 years ago

I left a couple of specific comments. If you fix those and also edit https://github.com/icambron/twix.js/blob/master/docs/docs.md, I'll merge this. Thanks for the contribution!

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.04%) to 97.927% when pulling 0f398544c2284215a0ca2fe0c943aa63cf9ace1f on IvanJov:master into a03ff817986a0dc4276c73d8861f7117083e9450 on icambron:master.

IvanJov commented 7 years ago

@icambron I've fixed those issues, sorry! Not so experienced with CoffeeScript, this is basically my first code written in it 😄 And re docs, what do you think, what's the best place to put toArray function?

icambron commented 7 years ago

You know, I'll do the docs, so don't worry about that. Thanks again!

IvanJov commented 7 years ago

@icambron Ok sure, thanks! I am glad I helped! If I come to something that can be useful I'll commit again!

icambron commented 7 years ago

@IvanJov Actually, now that I sit down to document it, is this really different from split?

> moment.utc('1982-05-25').twix(moment.utc('1982-05-27'), true).split(1, 'day').map(function(t){return t.start().format('YYYY-MM-DD');});
[ '1982-05-25', '1982-05-26', '1982-05-27' ]
IvanJov commented 7 years ago

@icambron Hmm, don't know, didn't think about split. I think that with toArray it's kinda easier to put range into array.

icambron commented 7 years ago

I'm going to leave it in. Releasing as 1.1.2