-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Convert tasks.js to yargs #210
Conversation
} | ||
case 'delete': { | ||
taskId = parseInt(input, 10); | ||
console.log('Task %d updated successfully.', options.taskId); |
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 logic go in the parent function (e.g. markDone
) as it did in the BigQuery samples, or in the yargs
handler?
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.
Probably should go in the markDone
function.
} | ||
// [END delete_entity] | ||
|
||
// [START format_results] |
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.
Are the docs going to continue to work with this region tag deleted?
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.
They should - this tag didn't come up anywhere when I did a code search.
9504fd1
to
29774f9
Compare
@jmdobry should be good to review. |
return callback(err); | ||
} | ||
|
||
return callback(); |
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.
Add the following above this line: console.log('Task %d deleted successfully.', taskId);
Aside: this sample needs (unit) tests - but I'm assuming those will go in a separate PR. |
Current coverage is 17.52% (diff: 15.38%)
|
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
* updated CHANGELOG.md * updated package.json * updated samples/package.json
* updated CHANGELOG.md * updated package.json * updated samples/package.json
No description provided.