[DO NOT MERGE] Propose refined sample format #154

jmdobry · 2018-10-10T17:52:57Z

New format based on recent discussions on sample best practices.

Format examples:

Basic - in this PR
File I/O - in this PR
LRO - [DO NOT MERGE] Propose refined sample format nodejs-speech#311

Notable changes

Each sample is now in its own file (lends itself well to auto-generated samples).
Added Node.js v8+ features such as spread, array destructuring, and async/await.
- Adding async/await necessitated adding a wrapper function inside the sample.
The most important variables (i.e. those that can't be hardcoded and must be set by the user) are commented out at the top of the sample.
The less important variables (request options with valid default values) are inlined into the request object.
Stripped out the yargs CLI wrapper in lieu of a simple ...process.argv.slice(2) which gets passed to the main so the sample can still be invoked from the command-line.
- Auto-generated samples could add back in Yargs if they want

Structure of samples

Outside region tag:
1. Copyright header
2. 'use strict';
Inside region tag (everything in here is what the user sees on cloud.google.com):
1. (If applicable) Setup variables, e.g. if given filepath, read in the file's bytes into a variable to be added to the request object.
2. Import statements
3. Instantiate client library
4. async function usefulFunctionNameHere():
  1. Construct request
    1. Includes inline comments about what the options are for
  2. Make API call
  3. Print interesting stuff to console, then return the response.
  4. Handle error (print error then re-throw)
5. Call the usefulFunctionNameHere function, passing in the variables in 2i.
Outside region tag:
1. Call the wrapping main function, passing in ...process.argv.slice(2)

Notes

If proposed format is acceptable, this would serve as a template for updating all Node.js samples.
Why did I pick the two samples that I picked? One is a very basic sample, only the projectId can't be hard-coded. The second is a more complicated sample that requires a filepath (which subsequently requires the file's bytes be read into memory)

samples/dlp_inspect_file.js

samples/dlp_inspect_string.js

samples/dlp_inspect_file.js

samples/dlp_inspect_image_file.js

samples/system-test/inspect.test.js

jmdobry · 2019-03-04T19:31:01Z

samples/system-test/inspect.test.js

  const output = await tools.runAsync(
-    `${cmd} string "I'm Gary and my email is gary@example.com"`,
+    `node dlp_inspect_string.js ${PROJECT_ID} "I'm Gary and my email is gary@example.com"`,


All the sample tests in this repo have that problem.

fhinkel · 2018-11-08T13:00:15Z

If we use this for how Node samples should look like, we need to replace ava with mocha and semistandard with prettier and eslint.

sduskis · 2019-01-22T17:02:13Z

@jmdobry, what's the status of this PR?

jmdobry · 2019-01-22T17:20:39Z

We'll be finalizing it end of February at a team summit.

codecov · 2019-02-27T06:17:00Z

Codecov Report

Merging #154 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #154   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           4      4           
=====================================
  Hits            4      4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f1d069...47f8669. Read the comment docs.

samples/.eslintrc.yml

samples/dlp_inspect_image_file.js

fhinkel · 2019-02-28T13:00:07Z

samples/dlp_inspect_image_file.js

+const fs = require('fs');
+
+export async function inspectImageFile(
+  projectId = 'YOUR_PROJECT_ID',


We should never require the projectId. Best practice is to leverage the metadata server present on all GCP runtimes.

DLP is somewhat special as the request needs the project ID, users should not pass it in though but use dlp.getProjectId(), https://github.com/googleapis/nodejs-dlp/blob/master/src/v2/dlp_service_client.js#L268.

It's not actually that special, quite a few APIs require project ID in path parameters to specify the resource owner for billing (different from the inferred project ID used for authentication). Current guidance from the sample working group is to make project ID explicit in the case where it's a required value in the request body. For almost all samples however, the project ID used for authentication should be inferred from the environment (hence the use of const dlp = new DLP.DlpServiceClient(); in the sample)

samples/system-test/inspect.test.js

jmdobry · 2019-03-05T16:08:43Z

Note: The failing test is not related to this PR.

cause

samples/dlp_inspect_image_file.js

bcoe · 2019-03-11T17:58:30Z

samples/dlp_inspect_image_file.js

+  // [END dlp_inspect_image_file]
+}
+
+main(...process.argv.slice(2));


there's been discussion in the Node.js tooling working group about providing a slightly higher level abstraction on process.argv; potentially Node.js would provide an object.

I agree with the decision to drop yargs so that folks don't have a dependency, but I've definitely found it can be a bit confusing for folks as to why they need to perform a slice operation.

Yeah, I made this as simple as possible here while still allowing the sample to be runnable from the command-line. Users on cloud.google.com won't see the slice, they'll just be copying the code between the START and END blocks. I imagine @beccasaurus and co. might auto-generate a more interesting/helpful CLI wrapper.

Yep, we use yargs with --input_param="value" flags for all sample input parameters

bcoe

I really like this work to modernize code samples, especially the fact that you've broken everything out into its own files -- as you say this will be great for auto generation.

samples/dlp_list_info_types.js

samples/dlp_delete_job.js

fhinkel

👍

beccasaurus · 2019-03-21T17:20:06Z

samples/dlp_delete_job.js

@@ -0,0 +1,47 @@
+/**


This should be samples/dlp-delete-job.js per recent @JustinBeckwith PR changes request

All files _ --> -

what's the reason for that? seems it would be easier for our tooling to have the filename be the exact region tag string

=> @JustinBeckwith

A few things!

JavaScript developers tend to prefer dashes to underscores

The previous file names were really long and had duplicate data (like the project name...) in them

I am generally nervous about encoding metadata inside of file names

So that's why lodash is more popular than underscore.js 😛 I feel this could be answered with BigQuery

What previous file names?

The cloud.google.com use case for our samples (where filename doesn't matter) far outweighs the use case where the filename even matters, so we're making things harder for our tooling without much benefit. But if e.g. for Java we already have to have logic to translate region tags to pascal-cased file names, we might as well have more logic to translate region tags to kebab-cased file names for Node.js.

Yeah to be clear - this ain't a hill I want to die on. With no other constraints, I would prefer dashes and not encoding metadata in the file name. If that's a ton of extra work - then I'm not gonna raise a fuss. I trust y'all :)

beccasaurus · 2019-03-27T05:01:36Z

samples/system-test/dlp_delete_job.test.js

+'use strict';
+
+const {assert} = require('chai');
+const dlp = require('@google-cloud/dlp');


Should the test use this style...

const {DlpServiceClient} = require('@google-cloud/dlp');

beccasaurus · 2019-04-05T06:44:45Z

samples/system-test/dlp_delete_job.test.js

@@ -0,0 +1,59 @@
+/**


s/system-test/test/g

We should now have tests in ./samples/test and deprecate the phrase system-test for our sample tests.

( I think? ) => this is a refactor that @JustinBeckwith did today

( fwiw – I think ./samples/test is clean. and people understand it. it's idiomatic. )
•
( when I read "system-test" my brain takes a moment to wonder "what types of tests are these?" )
•
( i would support this 🙈 )

Yeah, I tend to just switch these to test as I go.

jmdobry · 2019-05-15T16:28:43Z

Canonical samples have been documented elsewhere. Thanks for review!

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 10, 2018

ghost assigned jmdobry Oct 10, 2018

realjordanna reviewed Oct 10, 2018

View reviewed changes

samples/dlp_inspect_file.js Outdated Show resolved Hide resolved

realjordanna reviewed Oct 10, 2018

View reviewed changes

samples/dlp_inspect_file.js Outdated Show resolved Hide resolved

realjordanna reviewed Oct 10, 2018

View reviewed changes

samples/dlp_inspect_file.js Outdated Show resolved Hide resolved

realjordanna reviewed Oct 10, 2018

View reviewed changes

samples/dlp_inspect_string.js Outdated Show resolved Hide resolved

JustinBeckwith reviewed Oct 10, 2018

View reviewed changes

samples/dlp_inspect_file.js Outdated Show resolved Hide resolved

samples/dlp_inspect_file.js Outdated Show resolved Hide resolved

jmdobry mentioned this pull request Oct 11, 2018

[DO NOT MERGE] Propose new sample format. googleapis/nodejs-speech#199

Closed

jmdobry force-pushed the sample-proposal branch 3 times, most recently from efe47c5 to 787b237 Compare October 12, 2018 18:16

ghost assigned JustinBeckwith Oct 16, 2018

fhinkel reviewed Oct 26, 2018

View reviewed changes

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

fhinkel reviewed Oct 26, 2018

View reviewed changes

samples/system-test/inspect.test.js Outdated Show resolved Hide resolved

fhinkel reviewed Oct 26, 2018

View reviewed changes

JustinBeckwith added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 26, 2019

yoshi-automation added the 🚨 This issue needs some love. label Feb 7, 2019

JustinBeckwith added the status: blocked Resolving the issue is dependent on other work. label Feb 8, 2019

yoshi-automation removed the 🚨 This issue needs some love. label Feb 8, 2019

JustinBeckwith suggested changes Feb 27, 2019

View reviewed changes

samples/.eslintrc.yml Outdated Show resolved Hide resolved

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

fhinkel reviewed Feb 28, 2019

View reviewed changes

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

fhinkel reviewed Feb 28, 2019

View reviewed changes

samples/system-test/inspect.test.js Outdated Show resolved Hide resolved

jmdobry force-pushed the sample-proposal branch from ee9a9b2 to ddd94d7 Compare March 5, 2019 15:37

jmdobry changed the title ~~[DO NOT MERGE] Propose new sample format.~~ [DO NOT MERGE] Propose refined sample format Mar 5, 2019

jmdobry force-pushed the sample-proposal branch from ddd94d7 to 229c5bc Compare March 5, 2019 15:58

jmdobry mentioned this pull request Mar 5, 2019

[DO NOT MERGE] Propose refined sample format googleapis/nodejs-speech#311

Closed

beccasaurus reviewed Mar 11, 2019

View reviewed changes

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

beccasaurus reviewed Mar 11, 2019

View reviewed changes

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

beccasaurus reviewed Mar 11, 2019

View reviewed changes

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

jmdobry force-pushed the sample-proposal branch from 6144768 to cabcf55 Compare March 11, 2019 17:30

beccasaurus reviewed Mar 11, 2019

View reviewed changes

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

beccasaurus reviewed Mar 11, 2019

View reviewed changes

samples/dlp_inspect_image_file.js Outdated Show resolved Hide resolved

bcoe reviewed Mar 11, 2019

View reviewed changes

jmdobry force-pushed the sample-proposal branch from cabcf55 to 5322d38 Compare March 11, 2019 18:05

bcoe reviewed Mar 11, 2019

View reviewed changes

beccasaurus reviewed Mar 11, 2019

View reviewed changes

samples/dlp_list_info_types.js Outdated Show resolved Hide resolved

jmdobry force-pushed the sample-proposal branch 2 times, most recently from bac400f to cf6666f Compare March 12, 2019 03:29

fhinkel reviewed Mar 12, 2019

View reviewed changes

samples/dlp_delete_job.js Outdated Show resolved Hide resolved

fhinkel approved these changes Mar 13, 2019

View reviewed changes

JustinBeckwith mentioned this pull request Mar 20, 2019

Samples for batch PDF and image annotation. googleapis/nodejs-vision#347

Merged

3 tasks

beccasaurus reviewed Mar 21, 2019

View reviewed changes

Propose new sample format.

c5e165e

jmdobry force-pushed the sample-proposal branch from cf6666f to c5e165e Compare March 21, 2019 21:27

beccasaurus reviewed Mar 27, 2019

View reviewed changes

beccasaurus mentioned this pull request Apr 3, 2019

docs(samples): Add samples for translate v3 beta googleapis/nodejs-translate#234

Merged

3 tasks

beccasaurus reviewed Apr 5, 2019

View reviewed changes

Merge branch 'master' into sample-proposal

47f8669

jmdobry closed this May 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] Propose refined sample format #154

[DO NOT MERGE] Propose refined sample format #154

jmdobry commented Oct 10, 2018 •

edited

This comment was marked as spam.

jmdobry Mar 4, 2019

fhinkel commented Nov 8, 2018

sduskis commented Jan 22, 2019

jmdobry commented Jan 22, 2019

codecov bot commented Feb 27, 2019 •

edited

fhinkel Feb 28, 2019

fhinkel Feb 28, 2019

jmdobry Mar 4, 2019

fhinkel Mar 7, 2019

jmdobry commented Mar 5, 2019

bcoe Mar 11, 2019

jmdobry Mar 11, 2019

beccasaurus Mar 11, 2019

bcoe left a comment

fhinkel left a comment

beccasaurus Mar 21, 2019

jmdobry Mar 21, 2019

beccasaurus Mar 25, 2019

JustinBeckwith Mar 25, 2019

jmdobry Mar 26, 2019

JustinBeckwith Mar 26, 2019

beccasaurus Mar 27, 2019

beccasaurus Apr 5, 2019

beccasaurus Apr 5, 2019

beccasaurus Apr 5, 2019

JustinBeckwith Apr 5, 2019

jmdobry commented May 15, 2019

[DO NOT MERGE] Propose refined sample format #154

[DO NOT MERGE] Propose refined sample format #154

Conversation

jmdobry commented Oct 10, 2018 • edited

Notable changes

Structure of samples

Notes

This comment was marked as spam.

Choose a reason for hiding this comment

fhinkel commented Nov 8, 2018

sduskis commented Jan 22, 2019

jmdobry commented Jan 22, 2019

codecov bot commented Feb 27, 2019 • edited

Codecov Report

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 Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

fhinkel left a 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

jmdobry commented May 15, 2019

jmdobry commented Oct 10, 2018 •

edited

codecov bot commented Feb 27, 2019 •

edited