Add new "quickstart" samples. #174

jmdobry · 2016-09-29T01:29:50Z

These are intended to be simple examples that explicitly show:

Importing the client library
Instantiating a client
Making a single API call
Printing the result

Product covered:

BigQuery
- - sample
- - test
Datastore
- - sample
- - test
Language
- - sample
- - test
Logging
- - sample
- - test
Pub/Sub
- - sample
- - test
Speech
- - sample
- - test
Storage
- - sample
- - test
Translate
- - sample
- - test
Vision
- - sample
- - test

For reference, see the corresponding samples in other languages:

GoogleCloudPlatform/nodejs-docs-samples (samples are on master in various quickstart.js files)
Add new "quickstart" samples python-docs-samples#547
Add new "quickstart" samples. ruby-docs-samples#77
Add new "quickstart" samples. golang-samples#131
GoogleCloudPlatform/java-docs-samples (pending)
GoogleCloudPlatform/dotnet-docs-samples (pending)

Thanks!

jmdobry · 2016-09-29T01:31:20Z

How do you want these quickstart.php files organized? I've just been putting them into their own quickstart folder within each product folder, e.g.:

bigquery/
  quickstart/
    composer.json
    composer.lock
    quickstart.php

tmatsuo · 2016-09-29T22:51:41Z

bigquery/quickstart/quickstart.php

+$projectId = 'YOUR_PROJECT_ID';
+
+# Instantiates a client
+$bigqueryClient = new BigQueryClient([


The google-cloud-php can guess the projectId most of the cases.
I would omit the projectId, which makes the code more testable.

Same for the rest

The quickstarts should follow the same rules as the samples, and rely on the implicitly set project ID, rather than setting it explicitly, as this should be handled by the Client Libraries page.

tmatsuo · 2016-09-29T22:53:56Z

datastore/quickstart/quickstart.php

+$taskKey = $datastoreClient->key($kind, $id);
+
+# Retrieves the entity
+$dataset = $datastoreClient->lookup($taskKey);


$dataset -> $entity

Considering it's the first example for users, I don't think lookup is appropriate, because it will return null for the first time users.

bshaffer · 2016-09-29T23:33:48Z

datastore/quickstart/quickstart.php

+$projectId = 'YOUR_PROJECT_ID';
+
+# Instantiates a client
+$datastoreClient = new DatastoreClient([


$datastoreClient => $datastore for consistency with our other samples.

… comments

bshaffer · 2016-09-30T00:34:29Z

@tmatsuo PTAL

tmatsuo · 2016-09-30T00:45:58Z

bigquery/quickstart/test/quickstartTest.php

+        // Make sure it looks correct
+        $this->assertInstanceOf('Google\Cloud\BigQuery\Dataset', $dataset);
+        $this->assertEquals($datasetId, $dataset->id());
+        $dataset->delete();


Is it more appropriate to call delete() in tearDown or something?

tmatsuo · 2016-09-30T00:46:51Z

datastore/quickstart/quickstart.php


 # Retrieves the entity
-$dataset = $datastoreClient->lookup($taskKey);
+$entity = $datastore->entity($taskKey);


This doesn't retrieve the entity, it just creates a local Entity object. No API calls are made.

you're right. What do you think we should do for this test?

I'd create the key with named id and upsert the entity.
But I think we should match with the other language samples.

The other language samples do what this sample does. But I agree this is not a good sample. @jmdobry thoughts on changing this to an upsert?

tmatsuo · 2016-09-30T00:48:35Z

datastore/quickstart/quickstart.php

-$datastoreClient = new DatastoreClient([
-    'projectId' => $projectId
-]);
+$datastore = new DatastoreClient();

 # The kind of the entity to retrieve
 $kind = 'Task';
 # The id of the entity to retrieve
 $id = 1234567890;


It's a bit controversial to manually assign the numeric id (since you need to allocateId to avoid the collision with auto assigned id).

If we call "lookup" instead of "upsert" here, and maybe use a name instead of ID, would that be more appropriate?

$kind = 'Person'; $name = 'Bob'; $key = $datastore->key($kind, $name); $entity = $datastore->lookup($key);

tmatsuo · 2016-09-30T00:49:01Z

datastore/quickstart/test/quickstartTest.php

+    {
+        $file = sys_get_temp_dir() . '/datastore_quickstart.php';
+        $contents = file_get_contents(__DIR__ . '/../quickstart.php');
+        $contents = str_replace(


I don't think you need to replace anything

__DIR__ needs replaced for the autoloading to work

If the __DIR__ is the only thing you need to replace, you don't need to copy the file.

ahh, good point

tmatsuo · 2016-09-30T00:49:38Z

logging/quickstart/quickstart.php

+]);
+
+# List logging entries
+$entries = $logging->entries();


Nit: this will give you nothing, right? Is it appropriate for quickstart?

I am assuming there's at least something in the logs for their project, since this requests the logs without any filter. What do you suggest instead?

This is now fixed - the logging quickstart writes a logging entry

tmatsuo · 2016-09-30T00:50:06Z

logging/quickstart/quickstart.php

+
+# Instantiates a client
+$logging = new LoggingClient([
+    'projectId' => $projectId


I think we can remove $projectId here

My understanding was Logging was the one API we wanted to pass Project ID in for.

For the CLI, we still need an explicit projectId, but for creating the LoggingClient and do some simple stuff, it can be omitted.

Why do we need to pass in a Project ID for the CLI?

removed Project ID from quickstart

tmatsuo · 2016-09-30T00:50:45Z

logging/quickstart/test/quickstartTest.php

+        $file = sys_get_temp_dir() . '/logging_quickstart.php';
+        $contents = file_get_contents(__DIR__ . '/../quickstart.php');
+        $contents = str_replace(
+            ['YOUR_PROJECT_ID', '__DIR__'],


tmatsuo · 2016-09-30T00:51:15Z

pubsub/quickstart/test/quickstartTest.php

+        $topicName = 'my-new-topic-' . time();
+        $file = sys_get_temp_dir() . '/pubsub_quickstart.php';
+        $contents = file_get_contents(__DIR__ . '/../quickstart.php');
+        $contents = str_replace(


This one needs to copy the file so we can verify the new topic is created!

jmdobry · 2016-09-30T00:59:05Z

The quickstarts should follow the same rules as the samples, and rely on the implicitly set project ID, rather than setting it explicitly, as this should be handled by the Client Libraries page.

These samples are for the Client Libraries pages, therefore, can you bring back the explicit project IDs? These quickstart samples (which will only be shown on the Client Libraries pages) should be the only ones that use an explicit project ID. All other samples (as in, everything but the quickstart samples) should infer project ID.

Thanks for writing the tests!

Also, the various quickstart samples (Node.js, Ruby, Python) are nearly identical in terms of variable names, number of executable statement, comments in the code, etc., so if possible, it would be best to keep these PHP versions of those quickstart samples as close to the Node.js, Python, Ruby quickstart samples as possible.

We have specific requests from UX/PM for these samples to look a certain way, use certain comments, etc. I think it's okay for the quickstart samples to be "quirky" (they're only for the Client Libraries pages), meaning they don't follow the exact same conventions as the rest of your samples.

For example:

Node.js

// Your Google Cloud Platform project ID
const projectId = 'YOUR_PROJECT_ID';

// Instantiates a client
const bigqueryClient = BigQuery({ projectId: projectId });

Python

# Your Google Cloud Platform project ID
project_id = 'YOUR_PROJECT_ID'

# Instantiates a client
bigquery_client = BigQuery.Client(project=project_id)

Ruby

# Your Google Cloud Platform project ID
project_id = "YOUR_PROJECT_ID"

# Instantiates a client
gcloud = Google::Cloud new project_id
bigquery_client = gcloud.bigquery

PHP

# Your Google Cloud Platform project ID
$projectId = 'YOUR_PROJECT_ID';

# Instantiates a client
$bigqueryClient = new BigQueryClient([
    'projectId' => $projectId
]);

Yes, they're harder to test with an explicit project ID, but there shouldn't be too many of these types of sample.

jmdobry · 2016-09-30T01:00:17Z

Maybe "quickstart" was the wrong label for these samples?

tmatsuo · 2016-09-30T01:08:44Z

Yeah technically it's still testable with the replace hack...

What's the rationale of having projectIds?

jmdobry · 2016-09-30T01:20:22Z

What's the rationale of having projectIds?

@omaray?

bshaffer · 2016-09-30T01:22:59Z

My understanding is we would show both with and without the project ID being set, e.g. in the docs:

To instantiate the [PRODUCT] client:

<code>

If you need to explicitly set the project ID, you can pass it in like this:

<code>

If we do it this way (which I prefer), we can create another quickstart sample for each one create_client_with_project_id.php or something.

jmdobry · 2016-09-30T01:35:45Z

We, will just not on the same page. The quickstart samples, shown on Client Libraries pages, will use the explicit project ID. Everywhere else in the docs (the vast majority of samples) will use an implicit project ID.

jmdobry · 2016-09-30T01:37:16Z

logging/quickstart/quickstart.php

+]);
+
+# List logging entries
+$entries = $logging->entries();


Can we make this sample match the other logging quickstart samples (here's the python one), i.e. log an entry as opposed to listing entries.

bshaffer · 2016-09-30T01:47:40Z

Hmm, seems strange to recommend doing it two different ways. The docs can cover explicitly setting the project ID, but they should explain when this would be done (when application default credentials span multiple projects) and why.

bshaffer · 2016-09-30T01:48:39Z

As for using $bigqueryClient vs $bigquery, this is something we also want to be consistent with our other samples. I can't speak to the other languages, but I think it's more important to be consistent within the language docs than across languages.

…ickstart samples.

bshaffer · 2016-10-06T19:06:52Z

vision/quickstart/quickstart.php

+]);
+
+# The name of the image file to annotate
+$fileName = '/resources/wakeupcat.jpg';


We should add the __DIR__ here and not below.

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

speech/quickstart/quickstart.php

+]);
+
+# The name of the audio file to transcribe
+$fileName = '/resources/audio.raw';


We should add the __DIR__ here and not below.

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

speech/quickstart/quickstart.php

+# Detects speech in the audio file
+$results = $speech->recognize(fopen(__DIR__ . $fileName, 'r'), $options);
+
+echo 'Transcription: '.$results[0]['transcript'];


need spaces between concatenating .

bshaffer · 2016-10-06T22:51:10Z

datastore/quickstart/quickstart.php

 $kind = 'Task';

-# The name/ID of the entity to retrieve
+# The name/ID for the new entity
 $name = 'sampletask1';


"buymilktask"?

Nah, what would be the point of the description field in that case?

bshaffer · 2016-10-06T22:53:12Z

datastore/quickstart/quickstart.php

+# Saves the entity
+$datastore->upsert($task);
+
+echo 'Saved ' . $task->key() . ': ' . $task['description'];


This output will look like this:

Saved [ [Task: simpletask1] ]: Buy Milk

A little weird, but seems ok to me :D

I couldn't for the life of me get it to just print simpletask1 (which is what I did in other languages).

haha yeah, you would have to call $task->key()->path()[0]['name'], which isn't worth it

bshaffer · 2016-10-07T23:33:38Z

I believe this guy is ready for a final look!

jmdobry · 2016-10-07T23:42:18Z

LGTM except for one thing. The PHP Cloud Client library does not yet support auto-detecting the project ID, meaning that the user must set the GCLOUD_PROJECT environment variable for these samples to work. The docs will expect that all the user needs to do is gcloud beta auth application-default login. Is much work to make them all use an explicit project ID until the library supports auto-detecting the project ID?

…kstarts

jmdobry · 2016-10-08T04:01:12Z

Are composer.lock supposed to be committed to version control?

Also, I'm now seeing a number of files in this PR with changes (or being renamed or something) that are unrelated to the quickstart samples, was that intentional?

bshaffer · 2016-10-10T18:42:46Z

@jmdobry

composer.lock is committed to projects but not to dependencies. So this library should contain composer.lock (so the dependencies don't change w/o an explicit update) whereas a dependent library like google-cloud-php should never contain composer.lock

The renames had to happen for the organization of having quickstart not clutter up another sample application. I.e. logging/* became logging/api/* so that the logging sample code is clearly separated.

bshaffer · 2016-10-10T18:43:43Z

@tmatsuo PTAL

jmdobry · 2016-10-10T20:39:18Z

Can we please keep the branch around until I can update the docs? We don't want them to 404.

bshaffer · 2016-10-10T21:54:30Z

@jmdobry should just take a second to update these. I have the change in flight.

Add Pub/Sub quickstart sample.

331c184

jmdobry self-assigned this Sep 29, 2016

This was referenced Sep 29, 2016

Add new "quickstart" samples. GoogleCloudPlatform/ruby-docs-samples#77

Merged

Add new "quickstart" samples GoogleCloudPlatform/python-docs-samples#547

Merged

tmatsuo reviewed Sep 29, 2016

View reviewed changes

bshaffer reviewed Sep 29, 2016

View reviewed changes

adds tests for quickstarts, adds logging quickstart, and fixes review…

78c821b

… comments

bshaffer changed the title ~~[DO NOT MERGE] Add "quickstart" samples.~~ Add "quickstart" samples. Sep 30, 2016

tmatsuo reviewed Sep 30, 2016

View reviewed changes

jmdobry commented Sep 30, 2016

View reviewed changes

bshaffer added 10 commits September 29, 2016 18:59

updates logging quickstart to log entry

16982ac

Merge branch 'master' into quickstarts

8196545

moves logging to api subdirectory

0158ed9

moves dataset->delete to teardown

09e2cb1

adds composer autoloading to quickstarts

9340a8b

updates datastore quickstart to use query instead of entity

ac14b65

updates datastore sample to use lookup

27e54e5

removes project ID from logging, skips tests if ADC is not set

fd64f8b

moves topic deletion to teardown

124566c

fixes cs

59f9085

jmdobry changed the title ~~Add "quickstart" samples.~~ Add new "quickstart" samples. Oct 5, 2016

bshaffer and others added 3 commits October 5, 2016 00:23

adds project ID and tests to pubsub sample

21eaaa6

changes datastore entity back to Task

d187637

Added missing print statements. Added Language, Speech, and Vision qu…

8850d29

…ickstart samples.

bshaffer reviewed Oct 6, 2016

View reviewed changes

jmdobry added 2 commits October 6, 2016 12:51

Address comments.

dc00f27

Change datastore quickstart sample to create a task.

b7bda05

bshaffer reviewed Oct 6, 2016

View reviewed changes

jmdobry mentioned this pull request Oct 7, 2016

Add google-cloud-java client libraries quickstart samples. GoogleCloudPlatform/java-docs-samples#353

Merged

12 tasks

jmdobry and others added 3 commits October 7, 2016 10:29

Add missing print statment.

a772cd1

adds tests for language/speech/vision quickstarts

ff6a9c4

updates all quickstarts to use google-cloud 0.10

eda129e

adds projectId back to bigquery, storage, logging, and datastore quic…

4a2c81f

…kstarts

Merge branch 'master' into quickstarts

bc4e224

bshaffer assigned tmatsuo Oct 10, 2016

tmatsuo approved these changes Oct 10, 2016

View reviewed changes

bshaffer merged commit 627f249 into master Oct 10, 2016

bshaffer deleted the quickstarts branch October 10, 2016 19:52

jmdobry restored the quickstarts branch October 10, 2016 20:38

bshaffer deleted the quickstarts branch December 13, 2016 18:45

Add new "quickstart" samples. #174

Add new "quickstart" samples. #174

Conversation

jmdobry commented Sep 29, 2016 • edited by bshaffer Loading

jmdobry commented Sep 29, 2016

tmatsuo Sep 29, 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 commented Sep 30, 2016

Choose a reason for hiding this comment

tmatsuo Sep 30, 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

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

bshaffer Sep 30, 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

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

jmdobry commented Sep 30, 2016 • edited Loading

jmdobry commented Sep 30, 2016

tmatsuo commented Sep 30, 2016

jmdobry commented Sep 30, 2016

bshaffer commented Sep 30, 2016

jmdobry commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshaffer commented Sep 30, 2016

bshaffer commented Sep 30, 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

bshaffer Oct 6, 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 6, 2016 • edited Loading

Choose a reason for hiding this comment

jmdobry Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshaffer commented Oct 7, 2016

jmdobry commented Oct 7, 2016

jmdobry commented Oct 8, 2016

bshaffer commented Oct 10, 2016

bshaffer commented Oct 10, 2016

jmdobry commented Oct 10, 2016

bshaffer commented Oct 10, 2016

jmdobry commented Sep 29, 2016 •

edited by bshaffer

Loading

tmatsuo Sep 29, 2016 •

edited

Loading

tmatsuo Sep 30, 2016 •

edited

Loading

bshaffer Sep 30, 2016 •

edited

Loading

jmdobry commented Sep 30, 2016 •

edited

Loading

bshaffer Oct 6, 2016 •

edited

Loading

bshaffer Oct 6, 2016 •

edited

Loading

jmdobry Oct 6, 2016 •

edited

Loading