-
-
Notifications
You must be signed in to change notification settings - Fork 383
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
fix(#56): delete range of tasks #58
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
||
_generateNumbers(value) { | ||
let _start = value[0] || null; | ||
let _end = value[1] || null; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
[_start, _end] = [_end, _start]; | ||
} | ||
|
||
for (let i = parseInt(_start, 10); i <= parseInt(_end, 10); i++) { |
There was a problem hiding this comment.
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?
} | ||
|
||
_produceNumbers(values) { | ||
return [...new Set(this._flattenDown(values.map(value => value.indexOf('-') > -1 ? this._generateNumbers(value.split('-')) : value), []))]; |
There was a problem hiding this comment.
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.
return result; | ||
} | ||
|
||
_generateNumbers(value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename to _generateNumbersFromInterval
_generateNumbers(value) { | |
_generateNumbersFromInterval(value) { |
This PR solved #56.
Delete a range of tasks
For Example:
tb -d 1-5
ortb -d 1-5 7 9-11