-
Notifications
You must be signed in to change notification settings - Fork 25
feat(samples): add sample code for ListAssets v1p5beta1 #355
Conversation
Codecov Report
@@ Coverage Diff @@
## master #355 +/- ##
=======================================
Coverage 96.63% 96.63%
=======================================
Files 15 15
Lines 5532 5532
Branches 276 276
=======================================
Hits 5346 5346
Misses 180 180
Partials 6 6 Continue to review full report at Codecov.
|
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.
Noticed a couple things, let me know your thoughts.
samples/listAssets.js
Outdated
const projectResource = `projects/${projectId}`; | ||
// TODO(developer): Choose types of assets to list, such as 'storage.googleapis.com/Bucket': | ||
// const assetTypesList = ['storage.googleapis.com/Bucket', 'bigquery.googleapis.com/Table']; | ||
// Or simply use empty list to list all types of assets: |
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.
what we tend to do in comments here, is leave the expected variable commented out, such that someone could simply remove the //
and have a functional sample:
https://github.com/googleapis/nodejs-secret-manager/blob/master/samples/getSecretVersion.js#L22
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.
Got it. I changed the comments to use "const assetTypes = ..." directly, in order to align with the params used below.
samples/listAssets.js
Outdated
const result = await client.listAssets(request); | ||
// Handle the response. | ||
console.log(util.inspect(result, {depth: null})); | ||
// [END asset_quickstart_list_assets] |
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 believe this should be moved down to below listAssets()
;, such. that the code between START
and END
is a self contained functional block.
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.
Thanks for catching this! I was following the samples for other APIs. Now it seems the other samples need updating too.
🤖 I have created a release \*beep\* \*boop\* --- ## [3.5.0](https://www.github.com/googleapis/nodejs-asset/compare/v3.4.0...v3.5.0) (2020-07-08) ### Features * **samples:** add sample code for ListAssets v1p5beta1 ([#355](https://www.github.com/googleapis/nodejs-asset/issues/355)) ([1f4cef8](https://www.github.com/googleapis/nodejs-asset/commit/1f4cef8af558cc000aec52c4e92afc1774141c53)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #354 🦕