Skip to content

fix(#56): delete range of tasks#58

Open
rjoydip-zz wants to merge 2 commits intoklaudiosinani:masterfrom
rjoydip-zz:issue-56
Open

fix(#56): delete range of tasks#58
rjoydip-zz wants to merge 2 commits intoklaudiosinani:masterfrom
rjoydip-zz:issue-56

Conversation

@rjoydip-zz
Copy link
Copy Markdown

@rjoydip-zz rjoydip-zz commented Aug 4, 2018

This PR solved #56.

Delete a range of tasks

For Example: tb -d 1-5 or tb -d 1-5 7 9-11

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

@kalexmills kalexmills left a comment

Choose a reason for hiding this comment

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

lgtm

Comment thread lib/taskbook.js

_generateNumbers(value) {
let _start = value[0] || null;
let _end = value[1] || null;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should this still work if the user inputs taskbook -d 1-5-10?
Looks like it will currently just include 1-5, but maybe an error should be returned, or 1-10 should be used instead?

Copy link
Copy Markdown
Author

@rjoydip-zz rjoydip-zz Aug 12, 2018

Choose a reason for hiding this comment

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

You should use 1-10 instead of taskbook -d 1-5-10. That's more logical

Comment thread lib/taskbook.js
[_start, _end] = [_end, _start];
}

for (let i = parseInt(_start, 10); i <= parseInt(_end, 10); i++) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You use parseInt on both start and end twice, why not assign this one time and reuse?

Comment thread lib/taskbook.js
}

_produceNumbers(values) {
return [...new Set(this._flattenDown(values.map(value => value.indexOf('-') > -1 ? this._generateNumbers(value.split('-')) : value), []))];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I personally think that this line looks more complex than it really should be in the first look.
Why not improve this function to actually iterate in the values and generate a new array directly?
This way you will not need to flatten the array afterwards.

Comment thread lib/taskbook.js
return result;
}

_generateNumbers(value) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe rename to _generateNumbersFromInterval

Suggested change
_generateNumbers(value) {
_generateNumbersFromInterval(value) {

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