Skip to content
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

feat: Added "--after" option #52

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

WellerQu
Copy link

@WellerQu WellerQu commented Aug 4, 2018

We can use --after to filter the task data by date. like this:

tb --list done --after 20180804

or

tb --after 20180804

@@ -235,6 +238,16 @@ class Taskbook {
return data;
}

_filterAfterDate(date = moment().format(DATE_FORMATTER), data = this._data) {
Object.entries(data).forEach(([id, item]) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be re-written as the following?

Object.entries(data).filter(([id, item]) => {
  return moment(item._timestamp).isAfter(moment(date))
});

Copy link

@rjoydip-zz rjoydip-zz Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the filter is better than foreach for this case. Also deleting object is a little bit time expensive

Copy link
Author

@WellerQu WellerQu Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I can NOT, because the data is not an array, and I have to change the data directy in this project.

@knappg
Copy link

knappg commented Aug 7, 2018

We should probably update the help documentation as well. My understanding is that any ISO-8601 format should be parsed (https://momentjs.com/docs/#/parsing/string/) with your PR; not just the "20180806" format, which is pretty dang nifty!

@klaudiosinani klaudiosinani self-requested a review August 7, 2018 16:18
@klaudiosinani klaudiosinani added the enhancement New feature or request label Aug 7, 2018
@rjoydip-zz
Copy link

rjoydip-zz commented Aug 7, 2018

I think without using moment this could be possible in native way. Moment has 48-50kb lib size.

@ashinzekene
Copy link
Contributor

I would suggest using date-fns it is modular and its perf is better

https://medium.com/@k2u4yt/momentjs-vs-date-fns-6bddc7bfa21e

@knappg
Copy link

knappg commented Aug 8, 2018

It looks like date-fns also supports ISO-8601, so that's cool too. I think it's good to keep perf and size in mind, but it's worth mentioning that ~50kb is still fairly small when it comes to a globally downloaded node module like this that isn't being passed up and down the wire with every request.

Moment has a bit more usage and support than date-fns right now, but it looks like date-fns is being iterated on a bit more frequently than moment is.

@ashinzekene
Copy link
Contributor

I agree. I also think performance wise date-fns is better and I think that's important since tb takes some time to process operations

@WellerQu
Copy link
Author

WellerQu commented Aug 8, 2018

That's a good idea, I will replace moment with date-fns. @knappg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants