Update Datastore samples #249

jmdobry · 2016-11-08T22:56:36Z

Fixes #48
Fixes #234

Brings the code up to >= Node.js v4.3.2 ES2015 features

stephenplusplus · 2016-11-09T21:35:58Z

Should GCN be using new Node.js features in our docs as well?

jmdobry · 2016-11-09T22:45:19Z

That's up to you, though I would say in 4-6 months you're definitely safe changing your docs examples to use features that require Node.js >= v4.3.2

In these samples we're only adopting arrow functions, template strings, and const/let for now.

ace-n

(Disregard this comment)

ace-n · 2016-11-11T21:30:42Z

datastore/concepts.js

-  runQuery: function () {},
-  save: function () {}
+let datastore = {
+  delete: sinon.stub().returns(Promise.resolve([])),


Nit: is there a good way to make this less repetitive?

ace-n · 2016-11-15T19:25:18Z

datastore/concepts.js

+    datastore.get(taskKey)
+      .then((results) => {
+        // Task found.
+        const entity = results[0];


Nit: this block seems a bit long - is console.log(results[0]) too short?

That's how they wanted it

ace-n · 2016-11-15T20:55:07Z

datastore/concepts.js

-  this.datastore.runQuery(query, callback);
-};
+          return {
+            priorities: priorities,


Can we just do the following (per ES6), or is it not explicit enough?

return { priorities, percentCompletes }

I'm not a fan of doing this in user visible code, because I don't think it's the clearest ES2015 feature. If a user doesn't understand that feature, they'll just see a syntax error.

ace-n · 2016-11-15T21:08:59Z

datastore/concepts.js

+            // the `endCursor` property.
+            return runPageQuery(info.endCursor)
+              .then((results) => {
+                results[0] = entities.concat(results[0]);


This line was confusing at first - perhaps add a comment explaining it? (e.g. Add current results to results list or something like that.)

ace-n · 2016-11-15T21:10:39Z

datastore/concepts.js

+        const entities = results[0];
+        assert.equal(Array.isArray(entities), true);
+        const info = results[1];
+        if (!info || !info.endCursor) {


Is info.endCursor going to be set if no more results are available?

Yes. It will have a field that says "NO_MORE_RESULTS"

ace-n · 2016-11-15T21:12:20Z

datastore/concepts.js


-      transaction.save(accounts);
+  testEventualConsistentQuery () {


What's the point of this?

They show it in the docs...

ace-n · 2016-11-15T21:53:14Z

datastore/concepts.js

-    }
-    callback();
-  });
+module.exports = {


Use ES6 to shorten this? (e.g. Entity: Entity --> Entity)

No, see #249 (comment)

ace-n · 2016-11-15T21:59:33Z

datastore/system-test/concepts.test.js

+describe(`datastore:concepts`, () => {
+  before(() => {
+    const projectId = process.env.GCLOUD_PROJECT;
+    assert(projectId, `You must set the GCLOUD_PROJECT env var!`);


Does this conform to our standards?

Now it does.

ace-n · 2016-11-15T22:00:41Z

datastore/system-test/concepts.test.js

+
+  describe(`Transactions`, () => {
+    it(`performs a transactional update`, () => transaction.testTransactionalUpdate());
+    it(`performs retries if necessary`, () => transaction.testTransactionalRetry());


This pattern probably needs to be changed to fit our existing test standards - if not now, then later.

ace-n · 2016-11-15T22:15:58Z

datastore/tasks.js

+      const task = results[0];
+      task.done = true;
+      transaction.save({
+        key: taskKey,


Nit: can this be compressed by using ES6 and renaming variables? e.g. key: taskKey ==> key

No, see #249 (comment)

ace-n · 2016-11-15T22:20:33Z

datastore/tasks.js

-  deleteEntity: deleteTask,
-  main: function (args) {
+const program = module.exports = {
+  addTask: addTask,


Nit: can this be compressed by using ES6 and renaming variables? e.g. `markDone:

codecov-io · 2016-11-15T22:58:25Z

Current coverage is 94.50% (diff: 93.35%)

No coverage report found for master at 073fe3e.

Powered by Codecov. Last update 073fe3e...6b7cb36

jmdobry · 2016-11-16T17:33:59Z

Test failure is unrelated, something to do with a Pub/Sub sample.

ace-n · 2016-11-17T21:50:48Z

LGTM once CircleCI is passing and branch is up-to-date.

Fixes #234 Fixes #250 Addressed comments.

* chore(main): release 4.0.4 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

* updated CHANGELOG.md [ci skip] * updated package.json [ci skip] * updated samples/package.json Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

* chore(main): release 4.0.4 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

jmdobry assigned ace-n Nov 8, 2016

jmdobry mentioned this pull request Nov 9, 2016

@google-cloud/datastore 0.5.0 does not return entity keys with query results #250

Closed

ace-n reviewed Nov 15, 2016

View reviewed changes

jmdobry force-pushed the datastore branch 3 times, most recently from 63fd52c to 6b7cb36 Compare November 15, 2016 21:43

ace-n reviewed Nov 15, 2016

View reviewed changes

jmdobry force-pushed the datastore branch from 7f6a2c8 to 6bbf7f0 Compare November 16, 2016 22:57

jmdobry force-pushed the datastore branch from 6bbf7f0 to dbb186a Compare November 18, 2016 15:54

Fixes #48

ed81bc1

Fixes #234 Fixes #250 Addressed comments.

jmdobry force-pushed the datastore branch from dbb186a to ed81bc1 Compare November 21, 2016 22:31

jmdobry merged commit 1b3dd3f into master Nov 21, 2016

jmdobry deleted the datastore branch November 21, 2016 23:13

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

Fixes #48 (#249)

3cdf0c9

Fixes #234 Fixes #250 Addressed comments.

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

Fixes #48 (#249)

5036529

Fixes #234 Fixes #250 Addressed comments.

unforced mentioned this pull request Nov 17, 2022

migrate code from googleapis/nodejs-service-directory #2863

Merged

unforced mentioned this pull request Nov 17, 2022

migrate code from googleapis/nodejs-security-center #2867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Datastore samples #249

Update Datastore samples #249

jmdobry commented Nov 8, 2016

stephenplusplus commented Nov 9, 2016 •

edited

Loading

jmdobry commented Nov 9, 2016

ace-n left a comment •

edited

Loading

ace-n Nov 11, 2016

jmdobry Nov 16, 2016

ace-n Nov 15, 2016

jmdobry Nov 16, 2016

ace-n Nov 15, 2016

jmdobry Nov 16, 2016

ace-n Nov 15, 2016

jmdobry Nov 16, 2016

ace-n Nov 15, 2016

jmdobry Nov 16, 2016

ace-n Nov 15, 2016

jmdobry Nov 16, 2016

ace-n Nov 15, 2016

jmdobry Nov 16, 2016

ace-n Nov 15, 2016

jmdobry Nov 16, 2016

ace-n Nov 15, 2016

ace-n Nov 15, 2016

jmdobry Nov 16, 2016

ace-n Nov 15, 2016

codecov-io commented Nov 15, 2016 •

edited

Loading

jmdobry commented Nov 16, 2016

ace-n commented Nov 17, 2016

Update Datastore samples #249

Update Datastore samples #249

Conversation

jmdobry commented Nov 8, 2016

stephenplusplus commented Nov 9, 2016 • edited Loading

jmdobry commented Nov 9, 2016

ace-n left a comment • edited Loading

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

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

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

codecov-io commented Nov 15, 2016 • edited Loading

Current coverage is 94.50% (diff: 93.35%)

jmdobry commented Nov 16, 2016

ace-n commented Nov 17, 2016

stephenplusplus commented Nov 9, 2016 •

edited

Loading

ace-n left a comment •

edited

Loading

codecov-io commented Nov 15, 2016 •

edited

Loading