Add browsing data sample #201
Conversation
|
This branch didn't get rebased properly, it overwrites a lot of the copyTable code. |
| @@ -252,7 +266,7 @@ cli | |||
| .command('create <dataset> <table>', 'Create a new table in the specified dataset.', {}, function (options) { | |||
| program.createTable(utils.pick(options, ['dataset', 'table']), utils.makeHandler()); | |||
| }) | |||
| .command('list <dataset>', 'List tables in the specified dataset.', {}, function (options) { | |||
| .command('tables <dataset>', 'List tables in the specified dataset.', {}, function (options) { | |||
There was a problem hiding this comment.
This should stay list to be consistent with all the other sample CLIs.
There was a problem hiding this comment.
So the problem is that we can list both tables and rows - should I use something like list_tables and list_rows instead?
There was a problem hiding this comment.
I was thinking of having list for listing tables, since the sample is tables.js, and browse for the rows, which matches the sample description "Browse a table.".
node tables list my_dataset
node tables browse my_dataset my_table
There was a problem hiding this comment.
Or
node tables list my_dataset
node tables rows my_dataset my_table
or
node tables getRows my_dataset my_table
or
node tables get-rows my_dataset my_table
or
node tables listRows my_dataset my_table
or
node tables list-rows my_dataset my_table
I don't really mind what the CLI command is for running listRows, but I think the command for listing tables should be list for consistency.
There was a problem hiding this comment.
Point taken, will fix.
Current coverage is 89.46% (diff: 100%)@@ master #201 diff @@
==========================================
Files 57 57
Lines 2423 2431 +8
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 2164 2175 +11
+ Misses 259 256 -3
Partials 0 0
|
| // Delete bucket | ||
| setTimeout(function () { | ||
| storage.bucket(options.bucket).delete(done); | ||
| }, 2000); |
There was a problem hiding this comment.
Why these changes? Don't we still need to delete both srcDataset and destDataset?
|
LGTM after comments are addressed. |
|
@dpebot merge when green |
|
Okay! I'll merge when all statuses are green. |
dpebot
added
the
automerge
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: - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/nodejs-retail/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) Fixes #<issue_number_goes_here> 🦕
No description provided.