Adding snippets for Datastore queries #181

tmatsuo · 2016-10-06T19:28:24Z

No description provided.

tmatsuo · 2016-10-06T21:50:20Z

The datastore index!

I created the datastore index manually.

$ gcloud preview datastore --project cloud-samples-tests-php create-indexes index.yaml

bshaffer · 2016-10-06T21:59:11Z

datastore/api/src/functions/concepts.php

+{
+    // [START run_query]
+    $result = $datastore->runQuery($query);
+    // Result is a generator of \Google\Cloud\Datastore\Entity


I don't really feel this line is necessary, but if you feel strongly about it, it should be above the line of code and not below

Not necessary because @return states it's a generator

Hm, when you look at the source code on github, yes, it's obvious. But if you look at our docs, it's not.

But I will actually remove the comment. It's easy to guess the result is a list (or generator) and you can loop it with foreach anyways.

bshaffer · 2016-10-07T18:38:27Z

datastore/api/test/ConceptsTest.php

+            try {
+                $func();
+                return;
+            } catch (\Exception $e) {


We should make this PHPUnit_Framework_ExpectationFailedException

I can do that in this repo, but when we generalize it, I don't think it will work with phpunit 5.x

bshaffer · 2016-10-07T19:06:24Z

datastore/api/test/ConceptsTest.php

+
+    public function runEventualConsistentTest(Callable $func)
+    {
+        // TODO: Generalize and move it to php-tools.


See GoogleCloudPlatform/php-tools#15

this can be pulled in with php-tools v0.5.0

tmatsuo · 2016-10-11T00:06:01Z

The tests should fail now, but they should pass once the following PRs are submitted.
googleapis/google-cloud-php#194
googleapis/google-cloud-php#200

(of course, and also we update the google-cloud-php)

tmatsuo · 2016-10-11T17:43:09Z

Now the tests are passing with dev-master branch of the client library :)
I have squashed, rebased, and pushed. Let's see the tests are now stable with our eventual consistent test runner.

@bshaffer PTAL

tmatsuo · 2016-10-11T17:57:52Z

Oh logging test is failing

1) Google\Cloud\Samples\Logging\Tests\ListEntriesCommandTest::testListEntries

I will use the eventual consistent test runner for them too in a different PR: #186

bshaffer · 2016-10-11T20:00:28Z

datastore/api/composer.json

@@ -1,9 +1,10 @@
 {
    "require": {
-        "google/cloud": "dev-master"
+        "google/cloud": "<2.0"


Would prefer something closer to semantic versioning here, i.e. ~0.5

It might look weird, but it makes sense to me.

If we pin to the current version, something like 0.10.2 for example, in the future, users who copies this sample will use that pinned version, which might be outdated at the time when they start their project.

If we use something like ~0.10 it works and fetches the current version (0.10.2) now, but when google/cloud 0.11 is released, you will start with the outdated version.

The above statement is incorrect. It will fetch the newer version up to 1.0 (exclusive). It will still be outdated version when google/cloud 1.0 is released.

Unfortunately ~1.0 and ^1.0 don't work because the latest version of google/cloud is still 0.10.2.

Is it more intuitive if I do "~1.0|~0.10" or something like that?

tmatsuo · 2016-10-11T21:31:50Z

@bshaffer PTAL
I changed it to forms like "~1.0|~0.10"

bshaffer · 2016-10-12T17:46:24Z

datastore/api/src/functions/concepts.php

@@ -60,9 +61,9 @@ function create_entity(DatastoreClient $datastore)
 * @param DatastoreClient $datastore
 * @return \Google\Cloud\Datastore\Entity
 */
-function upsert_entity(DatastoreClient $datastore)
+function upsert(DatastoreClient $datastore)


I prefer upsert_entity to upsert, and same for below functions. What is the reason for changing it?

bshaffer · 2016-10-12T17:46:54Z

datastore/api/src/functions/concepts.php

@@ -256,7 +257,7 @@ function create_key_with_multi_level_parent(DatastoreClient $datastore)
 * @param Key $key
 * @return \Google\Cloud\Datastore\Entity
 */
-function create_entity_with_option(DatastoreClient $datastore, Key $key)
+function properties(DatastoreClient $datastore, Key $key)


Again, I think these functions (especially this one and array_value below) are being named too generically. I prefer the older names.

Specifically it matches to the region tags. This pattern matches with nodejs samples.

I was worried that would be the case :-/

Do you really need to change it? The function names here aren't shown to users

bshaffer · 2016-10-12T17:47:31Z

datastore/api/src/functions/concepts.php

+ * @param Key $lastSeenKey
+ * @return Query
+ */
+function kindless_query(DatastoreClient $datastore, Key $lastSeenKey)


kindless query? should this be keyless query?

As in Kind. Got it 👍

It also matches to other languages, specifically nodejs

bshaffer · 2016-10-12T17:52:21Z

datastore/api/src/functions/concepts.php

+ * Run a given query.
+ *
+ * @param DatastoreClient $datastore
+ * @return \Generator <\Google\Cloud\Datastore\Entity>


We could add use Generator above so that the leading backslash isn't necessary. Same with use Google.

bshaffer · 2016-10-12T17:54:06Z

datastore/api/src/functions/concepts.php

@@ -21,6 +21,7 @@
 use Google\Cloud\Datastore\DatastoreClient;
 // [END datastore_use ]


nit extra whitespace

Snippets on: https://cloud.google.com/datastore/docs/concepts/queries Also renames for consistensy with other languages.

bshaffer reviewed Oct 6, 2016

View reviewed changes

bshaffer reviewed Oct 7, 2016

View reviewed changes

tmatsuo force-pushed the more-datastore branch from c4d12e7 to 0eddc3e Compare October 11, 2016 17:43

tmatsuo assigned bshaffer Oct 11, 2016

bshaffer reviewed Oct 11, 2016

View reviewed changes

tmatsuo force-pushed the more-datastore branch from 51bc831 to b710d07 Compare October 11, 2016 21:31

bshaffer reviewed Oct 12, 2016

View reviewed changes

bshaffer approved these changes Oct 12, 2016

View reviewed changes

Takashi Matsuo added 4 commits October 12, 2016 11:07

Adding snippets for Datastore queries

9416fdf

Snippets on: https://cloud.google.com/datastore/docs/concepts/queries Also renames for consistensy with other languages.

Pin google/cloud

accbd73

Use "~1.0|~0.10" forms instead of "<2.0"

dac3b87

code review

4a271b2

tmatsuo force-pushed the more-datastore branch from b710d07 to 4a271b2 Compare October 12, 2016 18:07

tmatsuo merged commit c239b76 into master Oct 12, 2016

tmatsuo deleted the more-datastore branch October 12, 2016 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding snippets for Datastore queries #181

Adding snippets for Datastore queries #181

tmatsuo commented Oct 6, 2016

tmatsuo commented Oct 6, 2016

bshaffer Oct 6, 2016

bshaffer Oct 7, 2016 •

edited

Loading

tmatsuo Oct 7, 2016 •

edited

Loading

bshaffer Oct 7, 2016

tmatsuo Oct 7, 2016

bshaffer Oct 7, 2016

bshaffer Oct 10, 2016

tmatsuo commented Oct 11, 2016 •

edited

Loading

tmatsuo commented Oct 11, 2016

tmatsuo commented Oct 11, 2016 •

edited

Loading

bshaffer Oct 11, 2016 •

edited

Loading

tmatsuo Oct 11, 2016 •

edited

Loading

tmatsuo Oct 11, 2016

tmatsuo Oct 11, 2016

tmatsuo commented Oct 11, 2016

bshaffer Oct 12, 2016

bshaffer Oct 12, 2016

tmatsuo Oct 12, 2016

bshaffer Oct 12, 2016 •

edited

Loading

tmatsuo Oct 12, 2016

bshaffer Oct 12, 2016

bshaffer Oct 12, 2016

tmatsuo Oct 12, 2016

bshaffer Oct 12, 2016 •

edited

Loading

tmatsuo Oct 12, 2016

bshaffer Oct 12, 2016

tmatsuo Oct 12, 2016

		@@ -21,6 +21,7 @@
		use Google\Cloud\Datastore\DatastoreClient;
		// [END datastore_use ]

Adding snippets for Datastore queries #181

Adding snippets for Datastore queries #181

Conversation

tmatsuo commented Oct 6, 2016

tmatsuo commented Oct 6, 2016

Choose a reason for hiding this comment

bshaffer Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

tmatsuo Oct 7, 2016 • 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

tmatsuo commented Oct 11, 2016 • edited Loading

tmatsuo commented Oct 11, 2016

tmatsuo commented Oct 11, 2016 • edited Loading

bshaffer Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

tmatsuo Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmatsuo commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshaffer Oct 12, 2016 • 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

bshaffer Oct 12, 2016 • 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

bshaffer Oct 7, 2016 •

edited

Loading

tmatsuo Oct 7, 2016 •

edited

Loading

tmatsuo commented Oct 11, 2016 •

edited

Loading

tmatsuo commented Oct 11, 2016 •

edited

Loading

bshaffer Oct 11, 2016 •

edited

Loading

tmatsuo Oct 11, 2016 •

edited

Loading

bshaffer Oct 12, 2016 •

edited

Loading

bshaffer Oct 12, 2016 •

edited

Loading