Simplify BigQuery samples according to our standard. #207

jmdobry · 2016-09-06T22:38:39Z

No description provided.

jmdobry · 2016-09-07T00:23:19Z

Same eslint const issue is plaguing this PR.

ace-n · 2016-09-07T20:10:36Z

bigquery/system-test/datasets.test.js

@@ -49,15 +52,16 @@ describe('bigquery:datasets', function () {
  describe('listDatasets', function () {
    it('should list datasets', function (done) {
      program.listDatasets(projectId, function (err, datasets) {
-        assert.ifError(err);
+        assert.equal(err, null);
        assert(Array.isArray(datasets));


Fix assert style?

ace-n · 2016-09-07T20:54:19Z

bigquery/tables.js

-    format: options.format,
-    gzip: options.gzip
-  };
+var Storage = require('@google-cloud/storage');


Is there a reason you're declaring this here and on line 170?

Yes, because this is an import. Imports should be as close to top-level statements as possible. In ES6, the new version of JavaScript, this is enforced by the new import syntax, which only works as a top-level statement.

Ah, good to know.

ace-n · 2016-09-07T21:16:59Z

bigquery/test/queries.test.js

-      assert.equal(example.mocks.job.getMetadata.calledOnce, true);
-      assert.deepEqual(example.mocks.job.getMetadata.firstCall.args.slice(0, -1), []);
+      assert.equal(example.mocks.job.on.calledTwice, true);
+      assert.deepEqual(example.mocks.job.on.firstCall.args.slice(0, -1), ['error']);


Should these tests depend on the order of the callbacks? (It seems like they do, at least currently...)

The sample is deterministic and the current assertions are about as restrictive/specific as it gets, and won't fail unless someone changes the sample. The only alternative would be looser assertions.

Fair enough - adding this feature would likely add arguably unnecessary complexity to the tests.

ace-n · 2016-09-07T21:33:26Z

bigquery/test/tables.test.js

-      assert.deepEqual(program.copyTable.firstCall.args.slice(0, -1),
-        [srcDataset, srcTable, destDataset, destTable]
-      );
+      assert.deepEqual(program.copyTable.firstCall.args.slice(0, -1), [srcDatasetId, srcTableId, destDatasetId, destTableId]);


Nit: not personally a fan of lines > 100 chars.

Perhaps in a separate PR we can consider switching from Semistandard to ESLint + JSCS to give us some configurability and better enforceability on our style?

ace-n · 2016-09-07T21:57:40Z

LGTM, but I think there's some App Engine dependency issue going on...

🤖 I have created a release \*beep\* \*boop\* --- ### [2.3.8](https://www.github.com/googleapis/nodejs-game-servers/compare/v2.3.7...v2.3.8) (2021-08-17) ### Bug Fixes * **build:** migrate to using main branch ([#206](https://www.github.com/googleapis/nodejs-game-servers/issues/206)) ([04998bf](https://www.github.com/googleapis/nodejs-game-servers/commit/04998bf4f24613792beb12fff9d1a14da795b74b)) * **deps:** google-gax v2.24.1 ([#208](https://www.github.com/googleapis/nodejs-game-servers/issues/208)) ([8462da4](https://www.github.com/googleapis/nodejs-game-servers/commit/8462da415e3df130356f70e34f97698628f48b2b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

jmdobry assigned ace-n Sep 6, 2016

Simplify BigQuery samples according to our standard.

cfbaeb3

jmdobry force-pushed the simplify branch from aaa4f77 to cfbaeb3 Compare September 7, 2016 19:55

ace-n reviewed Sep 7, 2016
View reviewed changes

Address comments

d776c00

ace-n reviewed Sep 7, 2016
View reviewed changes

Add region tag.

ceb1f66

ace-n reviewed Sep 7, 2016
View reviewed changes

Re-enable cache. Remove .travis.yml

4cbff6d

ace-n reviewed Sep 7, 2016
View reviewed changes

jmdobry merged commit b5f33bf into master Sep 7, 2016

jmdobry deleted the simplify branch September 7, 2016 22:34

snyk-bot mentioned this pull request Jul 11, 2020

[Snyk] Fix for 1 vulnerabilities ninadhatkar/nodejs-docs-samples#2

Open

snyk-bot mentioned this pull request Aug 17, 2020

[Snyk] Fix for 1 vulnerabilities ninadhatkar/nodejs-docs-samples#21

Open

snyk-bot mentioned this pull request Aug 28, 2020

[Snyk] Fix for 1 vulnerabilities ninadhatkar/nodejs-docs-samples#25

Open

ninadhatkar mentioned this pull request Feb 23, 2021

[Snyk] Fix for 2 vulnerabilities ninadhatkar/nodejs-docs-samples#59

Open

pattishin mentioned this pull request Nov 9, 2022

migrate code from googleapis/nodejs-game-servers #2818

Closed

pattishin mentioned this pull request Nov 9, 2022

migrate code from googleapis/nodejs-game-servers #2819

Closed

pattishin mentioned this pull request Nov 9, 2022

migrate code from googleapis/nodejs-game-servers #2820

Closed

NimJay mentioned this pull request Nov 10, 2022

migrate code from googleapis/nodejs-language #2827

Merged

NimJay pushed a commit that referenced this pull request Nov 10, 2022

chore(deps): update dependency mocha to v6 (#207)

66d1f64

NimJay pushed a commit that referenced this pull request Nov 10, 2022

chore(deps): update dependency mocha to v6 (#207)

9bd5efe

pattishin mentioned this pull request Nov 11, 2022

migrate code from googleapis/nodejs-game-servers #2837

Merged

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

fix(deps): update dependency @google-cloud/storage to v4 (#207)

15a1c5b

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

fix(deps): update dependency @google-cloud/storage to v4 (#207)

dbe9d4e

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

fix(deps): update dependency @google-cloud/storage to v4 (#207)

56e437f

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

fix(deps): update dependency @google-cloud/storage to v4 (#207)

cb0d518

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

fix(deps): update dependency @google-cloud/storage to v4 (#207)

294716a

ahrarmonsur mentioned this pull request Nov 16, 2022

migrate code from googleapis/nodejs-asset #2856

Merged

ace-n pushed a commit that referenced this pull request Nov 17, 2022

fix(deps): update dependency @google-cloud/storage to v4 (#207)

0f83444

kweinmeister mentioned this pull request Nov 18, 2022

migrate code from googleapis/nodejs-text-to-speech #2872

Merged

kweinmeister pushed a commit that referenced this pull request Nov 18, 2022

refactor: use execSync for tests (#207)

944aa05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify BigQuery samples according to our standard. #207

Simplify BigQuery samples according to our standard. #207

jmdobry commented Sep 6, 2016

jmdobry commented Sep 7, 2016

ace-n Sep 7, 2016

jmdobry Sep 7, 2016

ace-n Sep 7, 2016

jmdobry Sep 7, 2016

ace-n Sep 7, 2016

ace-n Sep 7, 2016

jmdobry Sep 7, 2016

ace-n Sep 7, 2016

ace-n Sep 7, 2016 •

edited

Loading

jmdobry Sep 7, 2016

ace-n commented Sep 7, 2016

Simplify BigQuery samples according to our standard. #207

Simplify BigQuery samples according to our standard. #207

Conversation

jmdobry commented Sep 6, 2016

jmdobry commented Sep 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ace-n Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ace-n commented Sep 7, 2016

ace-n Sep 7, 2016 •

edited

Loading