New Command: todo task remove #1930
Conversation
|
Will do! Thank you for such a quick turnaround 👏 |
waldekmastykarz
left a comment
waldekmastykarz
left a comment
There was a problem hiding this comment.
Nicely done with a few things I've fixed when merging the PR 👍
| : The name of the task list in which the task exists. Specify either `listId` or `listName`, not both | ||
|
|
||
| `--listId [listId]` | ||
| : The id of the task list in which the task exists. `listId` or `listName`, not both |
There was a problem hiding this comment.
Missing piece of text: Specify either
|
|
||
| public getTelemetryProperties(args: CommandArgs): any { | ||
| const telemetryProps: any = super.getTelemetryProperties(args); | ||
| telemetryProps.id = typeof args.options.id !== 'undefined'; |
There was a problem hiding this comment.
Since id is required, there is no need to track it in the telemetry because it would always show up as set.
|
|
||
| public validate(args: CommandArgs): boolean | string { | ||
|
|
||
| if (!args.options.id) { |
There was a problem hiding this comment.
In the latest versions of the CLI, we no longer need to manually check for the presence of required options.
|
|
||
| public commandAction(logger: Logger, args: CommandArgs, cb: (err?: any) => void): void { | ||
|
|
||
| const getToDoListId = () => { |
There was a problem hiding this comment.
Let's be explicit about the returned types to avoid runtime errors.
| }; | ||
| return request | ||
| .get(requestOptions) | ||
| .then((response: any) => response.value && response.value.length === 1 ? response.value[0].id : null); |
There was a problem hiding this comment.
Let's avoid using any to avoid runtime errors
| id: "AAMkAGI3NDhlZmQzLWQxYjAtNGJjNy04NmYwLWQ0M2IzZTNlMDUwNAAuAAAAAACQ1l2jfH6VSZraktP8Z7auAQCbV93BagWITZhL3J6BMqhjAAD9pHIhAAA=", | ||
| listName: "Tasks" | ||
| } | ||
| } as any, () => { |
There was a problem hiding this comment.
There is no need to cast args as any
| listName: "Tasks", | ||
| confirm: true | ||
| } | ||
| } as any, () => { |
There was a problem hiding this comment.
There is no need to cast args as any
| } | ||
| } as any, () => { | ||
| try { | ||
| //assert(log[log.length-1].indexOf('DONE') > -1); |
There was a problem hiding this comment.
We shouldn't include commented out code
| } | ||
| } as any, () => { | ||
| try { | ||
| //assert(log[log.length-1].indexOf('DONE') > -1); |
There was a problem hiding this comment.
This test is missing an assert so it will always show up as passing
| let promptIssued = false; | ||
|
|
||
| if (promptOptions && promptOptions.type === 'confirm') { | ||
| promptIssued = true; |
There was a problem hiding this comment.
Here, you're checking the value of the promptIssued variable, that was set in previous tests. In this test, you're resetting prompt behavior on line 310, but you're not tracking promptOptions in here, so the value in this test is still from previous tests.
|
Merged manually. Thank you! 👍 |
|
@waldekmastykarz, Thnak you so much for your valuable feedbacks and time to review my code! |
2 participants
Hi Team,
Could you please review this and let me know if you have any questions?
Thanks