[Syntax Review] Additional types of SampleGen code samples by beccasaurus · Pull Request #7051 · googleapis/google-cloud-java

beccasaurus · 2019-12-17T20:52:06Z

Follow-up code review PR –

To evaluate the readiness of ☕️ Java's GAPIC sample generator for use on cloud.google.com,
this provides a full set of samples which demonstrate the basic features of generated samples.

The .yaml sample config files for these samples can all be found here: .yaml sample configs

The first review Pull Request contained samples to show the basic structure as well as LRO samples.

This contains samples demonstrating those features and a few more use-cases for review –

Sample	Description
`SamplegenBasics.java`	Shows basic structure/anatomy of Java samples
`SamplegenNoConfig.java`	Minimal sample example (zero configuration)
`SamplegenNoResponse.java`	API method which returns Empty
`SamplegenPaged.java`	Paged API method (iterates over pages of results)
`SamplegenResourcePath.java`	Sample which requires user's Google Project ID
`SamplegenMapFieldAccess.java`	Access map value by key and enumerate keys/values
`SamplegenReadAndWriteFiles.java`	Read binary file into bytes & write string to local file
`SamplegenRepeatedFields.java`	Repeated request field & show loop/access by index
`SamplegenWriteBytesToFile.java`	Write bytes field in response to local file
`SamplegenLro.java`	Call long-running operation API endpoint

This Pull Request is focused on feedback collection & a similar PR is being sent to all languages.

Once you've completed your review and left comments, you may feel free to close the PR!

We will scope out making all of the requested changes and reach out to language team offline.

🙏 Thanks in advance for your review!

P.S. Nits welcome! We'd like to get these as polished as possible for usage on cloud.google.com.

Please feel free to note in comments whether something is: 🛑 Blocker -vs- 📌 Nit

🔍 Known Issues

Switch commented out placeholder variables => provide an overload with defaults
❓ Possible to add link to available types for enums?
When using resources, add a comment with try block explaining importance
❓ Exception handling – can we show something better than catch (Exception)?
Remove “DO NOT EDIT” / soften language so that developers are the target audience

🔍 Tool for review: Java SAMPLE_FORMAT

📑 (internal)

Note: this is using the same sample .snip templates as with the previous PR #6543

beccasaurus · 2019-12-17T20:53:05Z

Hello Java friends ☕️

Follow-up PR, as promised!

@kurtisvg @averikitsch @lesv @chingor13 @googleapis/samplegen

beccasaurus · 2019-12-17T20:54:28Z

Note: pulling this together required generating samples from a variety of different APIs, hence PR being sent to google-cloud-java rather than to the individual java-* repos

beccasaurus · 2019-12-17T21:05:28Z

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


ℹ️ Note: this "no config" sample is different from the rest. Rather than demonstrating a feature, it shows what a sample looks like when not configured at all.

These samples could be generated for every API method without any manual configuration.

Review of the "no config" sample is not required, this is more of an FYI :)

lesv · 2019-12-17T21:06:56Z

+   * Please include the following imports to run this sample.
+   *
+   * import com.google.cloud.vision.v1.CreateProductSetRequest;
+   * import com.google.cloud.vision.v1.LocationName;
+   * import com.google.cloud.vision.v1.ProductSearchClient;
+   * import com.google.cloud.vision.v1.ProductSet;


We've already provided feedback - strike the comments.

nnegrey · 2019-12-17T21:09:17Z

+   * import com.google.cloud.vision.v1.ProductSet;
+   */
+
+  public static void sampleCreateProductSet() {


This sample has a constructor, but most do not?

nnegrey · 2019-12-17T21:12:48Z

+// sample-metadata:
+//   title: This is the sample title
+//   description: This is the sample description
+//   usage: gradle run -PmainClass=com.google.cloud.examples.vision.v1.SamplegenBasics [--args='[--display_name "This is the default value of the display_name request field"]']


I thought we typically used maven in samples?

I think the goal is that we should support both - but since for Java we expect most users to run these from the IDE (instead of command line) these instructions should be something along the lines of "Update the values under the TODO(developer) line and execute the function to run this sample).

nnegrey · 2019-12-17T21:16:43Z

+  /**
+   * This is the sample description
+   *
+   * @param displayName Description of the parameter
+   */


Believe no javadocs.
Also since the constructor has the displayName variable this is redundant.

codecov · 2019-12-17T21:17:13Z

Codecov Report

Merging #7051 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #7051      +/-   ##
============================================
- Coverage     32.18%   32.18%   -0.01%     
+ Complexity     3698     3697       -1     
============================================
  Files           249      249              
  Lines         40371    40371              
  Branches       6134     6134              
============================================
- Hits          12994    12992       -2     
- Misses        26304    26305       +1     
- Partials       1073     1074       +1

Impacted Files	Coverage Δ	Complexity Δ
...oogle/cloud/spanner/jdbc/SingleUseTransaction.java	`86% <0%> (-1%)`	`36% <0%> (-1%)`

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 8befeef...8d3e58c. Read the comment docs.

nnegrey · 2019-12-17T21:17:52Z

+   * @param displayName Description of the parameter
+   */
+  public static void sampleCreateProductSet(String displayName) {
+    try (ProductSearchClient productSearchClient = ProductSearchClient.create()) {


I believe we are also adding this comment to all client declarations:

// Initialize client that will be used to send requests. This client only needs to be created // once, and can be reused for multiple requests. After completing all of your requests, call // the "close" method on the client to safely clean up any remaining background resources.

I love this! +💯 Thanks for providing the exact comment!

nnegrey · 2019-12-17T21:19:24Z

+    try (ProductSearchClient productSearchClient = ProductSearchClient.create()) {
+
+      // The project and location in which the product set should be created.
+      LocationName parent = LocationName.of("[PROJECT]", "[LOCATION]");


Anything the user needs to set should be in the constructor

nnegrey · 2019-12-17T21:22:04Z

+import com.google.cloud.vision.v1.ProductSearchClient;
+import com.google.cloud.vision.v1.ProductSetName;
+
+public class SamplegenNoResponse {


No response or empty response?

lesv · 2019-12-17T21:09:01Z

+    try (ProductSearchClient productSearchClient = ProductSearchClient.create()) {
+
+      // The project and location in which the product set should be created.
+      LocationName parent = LocationName.of("[PROJECT]", "[LOCATION]");


nit - I wish we used EnvVar's for this rather than user editing.

There should probably be TODO(developer) here as well.

These would be good to pass in as arguments, so we can test them.

lesv · 2019-12-17T21:09:52Z

+
+  public static void sampleCreateProductSet() {
+    // TODO(developer): Replace these variables before running the sample.
+    String displayName = "This is the default value of the display_name request field";


nit - wish this was an EnvVar

I disagree on this one - if we expect users to copy/paste, it makes more sense that they would just update the values in the IDE after doing so.

It makes sense that the tests would look for EnvVars though.

lesv · 2019-12-17T21:12:45Z

+    } catch (Exception exception) {
+      System.err.println("Failed to create the client due to: " + exception);
+    }


BLOCK - I think we've already provided feedback on this.
Exceptions should be as specific as possible and suggest what to do. Just printing the error isn't good form.
https://google.github.io/styleguide/javaguide.html#s6.2-caught-exceptions

lesv · 2019-12-17T21:14:14Z

+  public static void main(String[] args) throws Exception {
+    Options options = new Options();
+    options.addOption(
+        Option.builder("").required(false).hasArg(true).longOpt("display_name").build());


BLOCK - "" ? This doesn't tell me much.
If I was reviewing, I'd certainly ask for a comment.

lesv · 2019-12-17T21:14:51Z

+  }
+  // [END samplegen_basics]
+
+  public static void main(String[] args) throws Exception {


BLOCK - Exceptions should ideally be very specific if thrown to tell the user what to expect.

Additionally - ArgParsing should be ideally outside the class (but at the very least excluded from the snippets).

lesv · 2019-12-17T21:41:56Z

+  public static void sampleCreateProductSet(String project) {
+    try (ProductSearchClient productSearchClient = ProductSearchClient.create()) {
+      LocationName parent = LocationName.of(project, "us-central1");
+      String displayName = "[DISPLAY NAME]";


nit - would prefer EnvVar or as a passed param.

lesv · 2019-12-17T21:44:33Z

+    } catch (Exception exception) {
+      System.err.println("Failed to create the client due to: " + exception);
+    }


BLOCK - should be specific exception(s).

lesv · 2019-12-17T21:45:16Z

+
+  public static void main(String[] args) throws Exception {
+    Options options = new Options();
+    options.addOption(Option.builder("").required(false).hasArg(true).longOpt("project").build());


Should explain ""

lesv · 2019-12-17T21:45:41Z

+  /*
+   * Please include the following imports to run this sample.
+   *
+   * import com.google.cloud.texttospeech.v1.AudioConfig;
+   * import com.google.cloud.texttospeech.v1.AudioEncoding;
+   * import com.google.cloud.texttospeech.v1.SynthesisInput;
+   * import com.google.cloud.texttospeech.v1.SynthesizeSpeechRequest;
+   * import com.google.cloud.texttospeech.v1.SynthesizeSpeechResponse;
+   * import com.google.cloud.texttospeech.v1.TextToSpeechClient;
+   * import com.google.cloud.texttospeech.v1.VoiceSelectionParams;
+   * import java.io.FileOutputStream;
+   * import java.io.OutputStream;
+   */


nit - STRIKE

lesv · 2019-12-17T21:47:00Z

+    } catch (Exception exception) {
+      System.err.println("Failed to create the client due to: " + exception);
+    }


BLOCK - needs to be the specific ERROR's so user knows to write code to deal with the situation, or don't catch them and add exception to the method defn.

averikitsch · 2019-12-18T18:40:32Z

+    try (ProductSearchClient productSearchClient = ProductSearchClient.create()) {
+
+      // The project and location in which the product set should be created.
+      LocationName parent = LocationName.of("[PROJECT]", "[LOCATION]");


These would be good to pass in as arguments, so we can test them.

averikitsch · 2019-12-18T18:42:29Z

+      ProductSet productSet = response;
+      System.out.printf("The full name of the created product set: %s\n", productSet.getName());
+    } catch (Exception exception) {
+      System.err.println("Failed to create the client due to: " + exception);


Question to the Java DPEs: Should we be using a better logging library like slf4j?

In the sample format, we already suggest using java.util.logging for web applications - slf4j matches this interface I think?

Maybe just expand this to suggesting for all samples?

(this is probably a nit)

averikitsch · 2019-12-18T18:47:09Z

+      String language = "en";
+
+      // The text content to analyze
+      String content = "Googleplex is at 1600 Amphitheatre Parkway, Mountain View, CA.";


Question for DPEs: a lot of the code in these samples feel squished together. Is that because of the formatting of builders?

IMO - there should be a breaks between building, sending/receiving, and acting on the response (Arrange, Act, Assert)

@beccasaurus, does samplegen support adding empty lines yet?

kurtisvg · 2019-12-18T20:47:53Z

+// sample-metadata:
+//   title: This is the sample title
+//   description: This is the sample description
+//   usage: gradle run -PmainClass=com.google.cloud.examples.vision.v1.SamplegenBasics [--args='[--display_name "This is the default value of the display_name request field"]']


I think the goal is that we should support both - but since for Java we expect most users to run these from the IDE (instead of command line) these instructions should be something along the lines of "Update the values under the TODO(developer) line and execute the function to run this sample).

kurtisvg · 2019-12-18T20:49:10Z

+
+  public static void sampleCreateProductSet() {
+    // TODO(developer): Replace these variables before running the sample.
+    String displayName = "This is the default value of the display_name request field";


I disagree on this one - if we expect users to copy/paste, it makes more sense that they would just update the values in the IDE after doing so.

It makes sense that the tests would look for EnvVars though.

kurtisvg · 2019-12-18T20:51:31Z

+      // The API response represents the created product set
+      ProductSet productSet = response;
+      System.out.printf("The full name of the created product set: %s\n", productSet.getName());
+    } catch (Exception exception) {


Should catch a more specific Exception - See Exception Handling

kurtisvg · 2019-12-18T20:54:34Z

+      ProductSet productSet = response;
+      System.out.printf("The full name of the created product set: %s\n", productSet.getName());
+    } catch (Exception exception) {
+      System.err.println("Failed to create the client due to: " + exception);


In the sample format, we already suggest using java.util.logging for web applications - slf4j matches this interface I think?

Maybe just expand this to suggesting for all samples?

kurtisvg · 2019-12-18T20:55:34Z

+  }
+  // [END samplegen_basics]
+
+  public static void main(String[] args) throws Exception {


Additionally - ArgParsing should be ideally outside the class (but at the very least excluded from the snippets).

kurtisvg · 2019-12-18T21:00:11Z

+      RecognitionAudio audio = RecognitionAudio.newBuilder().setUri(uri).build();
+      LongRunningRecognizeRequest request =
+          LongRunningRecognizeRequest.newBuilder().setConfig(config).setAudio(audio).build();
+      OperationFuture<LongRunningRecognizeResponse, LongRunningRecognizeMetadata> future =


nit: Ideally there would be a break between the section that builds the request and the section that sends the request - See Arrange, Act, Assert

Additionally, I think it would be helpful to include a comment explaining that the request is only to start the operation, which is why the future.get is called later.

kurtisvg · 2019-12-18T21:02:13Z

+  /** This sample reads and loops over a map field in the response */
+  public static void sampleAnalyzeEntities() {
+    try (LanguageServiceClient languageServiceClient = LanguageServiceClient.create()) {
+      Document.Type type = Document.Type.PLAIN_TEXT;


Similar to my comment in Python, we should provide links to docs or page on what values are supported.

kurtisvg · 2019-12-18T21:04:10Z

+      String language = "en";
+
+      // The text content to analyze
+      String content = "Googleplex is at 1600 Amphitheatre Parkway, Mountain View, CA.";


IMO - there should be a breaks between building, sending/receiving, and acting on the response (Arrange, Act, Assert)

kurtisvg · 2019-12-18T21:05:05Z

+        for (Map.Entry<String, String> entry : entity.getMetadataMap().entrySet()) {
+          String key = entry.getKey();
+          String value = entry.getValue();
+          System.out.printf("%s: %s\n", key, value);


nit: inline single use variables

Suggested change

System.out.printf("%s: %s\n", key, value);

System.out.printf("%s: %s\n", entry.getKey(), entry.getValue());

kurtisvg · 2019-12-18T21:08:57Z

+        }
+
+        // Loop over just keys:
+        for (String theKey : entity.getMetadataMap().keySet()) {


Similar to python - these two loops don't show any API interaction with the library, they are only showing basic Java syntax. Since we've already established it's a map in the first loop, these two feel unnecessary.

[SampleGen] Additional types of samples for review

8d3e58c

Note: this is using the same sample .snip templates as with the previous PR #6543

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 17, 2019

beccasaurus requested review from chingor13, kurtisvg and lesv December 17, 2019 21:03

beccasaurus commented Dec 17, 2019

View reviewed changes

lesv reviewed Dec 17, 2019

View reviewed changes

nnegrey reviewed Dec 17, 2019

View reviewed changes

lesv reviewed Dec 17, 2019

View reviewed changes

averikitsch reviewed Dec 18, 2019

View reviewed changes

kurtisvg suggested changes Dec 18, 2019

View reviewed changes

chingor13 closed this Dec 7, 2020

	System.out.printf("%s: %s\n", key, value);
	System.out.printf("%s: %s\n", entry.getKey(), entry.getValue());

Conversation

beccasaurus commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🙏 Thanks in advance for your review!

🔍 Known Issues

Uh oh!

beccasaurus commented Dec 17, 2019

Uh oh!

beccasaurus commented Dec 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beccasaurus commented Dec 17, 2019 •

edited

Loading

codecov Bot commented Dec 17, 2019 •

edited

Loading