java-docs-samples icon indicating copy to clipboard operation
java-docs-samples copied to clipboard

feat(scc): add v2 samples - muteconfig and findings

Open Sita04 opened this issue 1 year ago • 2 comments

Add new samples for Security Command Center v2 APIs.

Total: 12 samples (MuteConfig + Findings)

Blocked by library release.

Sita04 avatar Jan 25 '24 18:01 Sita04

QQ: we discussed to temporarily suspend adding new samples to the repo. What is necessity for these particular samples?

minherz avatar Jan 26 '24 05:01 minherz

@minherz Ack, but the samples are supporting a new launch. Hence, the priority in adding them.

Sita04 avatar Jan 29 '24 18:01 Sita04

Here is the summary of changes.

You are about to add 12 region tags.

This comment is generated by snippet-bot. If you find problems with this result, please file an issue at: https://github.com/googleapis/repo-automation-bots/issues. To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • [ ] Refresh this comment

snippet-bot[bot] avatar Mar 12 '24 12:03 snippet-bot[bot]

  • nit: it is generally recommended to narrow the scope of code snippets by placing the region tags around the actual code while omitting main() methods and other not relevant details. Mind that you can use more the region tag in more than place within the file so you can provide necessary import statements to ensure that copy-n-paste of the code snippet will compile.

  • nit: The best practice is considered to return actual values for code snippets that get information. It allows to write more robust unit tests.

  • nit: It would be a good practice either to catch the API related exception or explicitly declare them (using throws statement in the method declaration)

  • Inconsistency in the error handling; there are several considerations regarding it:

    • Best practices for error handling in Java
    • Expose what kind of thrown exceptions are related to this particular API or user scenarion
    • Support writing unit tests that validate specific errors (thrown exceptions)

Thanks for your review, Leonid!

  1. This guideline is not standard and different languages have their own implementation. In Java, we have primarily resorted to following "one snippet per class" rule. Will follow up on this discussion internally to see if it's time to update this!

  2. I've updated snippets that can benefit from returning results. Few snippets like listing and grouping findings, I've left the tests unchanged, primarily because this could mean passing unknown number of result records back to tests.

  3. For exceptions, I've updated the snippets to follow the guideline of catching only API related exceptions.

Sita04 avatar Mar 13 '24 15:03 Sita04

... Thanks for your review, Leonid!

  1. This guideline is not standard and different languages have their own implementation. In Java, we have primarily resorted to following "one snippet per class" rule. Will follow up on this discussion internally to see if it's time to update this!

I do not follow the reference about different languages here. The first bullet in the original comment explained that the code snippet (part of code enclosed by the region tag) should show only the relevant code to ensure that the documentation is not overloaded with extra content. The files in this PR use region tags in a way that code snippet includes the whole file with the exception of the copyright comment. It is not a blocker, but including the main() method into code snippet does not contribute to the example of the API call usage but only helps to run the code sample if user decides to.

  1. I've updated snippets that can benefit from returning results. Few snippets like listing and grouping findings, I've left the tests unchanged, primarily because this could mean passing unknown number of result records back to tests.

It looks like only one "get" code samples was modified. It is not a blocker, but I would recommend considering to modify all "get" code samples. The delete/set/update code samples can communicate the status as not throwing exception ==> OK. If any of the delete/set/update code samples receive the status of the operation success on response, they can return that status as well. There should be no concern regarding unknown number of results because the tests should validate the expected content. Currently they do not validate a number of results or all returned content either.

  1. For exceptions, I've updated the snippets to follow the guideline of catching only API related exceptions.

I do not see any catch statements in the files. Is it supposed to be like this?

minherz avatar Mar 14 '24 22:03 minherz