feat: inline disclaimer comment, use Sample/RegionTag, collect GapicClass samples (pt 2) #970
Conversation
product-auto-label
bot
added
the
samples
src/main/java/com/google/api/generator/gapic/composer/samplecode/SampleComposerUtil.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private static List<Sample> handleDuplicateSamples(List<Sample> samples) { |
There was a problem hiding this comment.
What is the scenario for "duplicate samples"? Is it for RPCs that have multiple samples? If it is, they are not really duplicate samples, they are overloaded samples for the same RPC. This name kind of implies we are removing duplicates, which could be misleading.
I see a few issues with the logic in this method:
- The logic is very complex and not easy to understand, please consider simplify it or break it down.
- This complex logic is being called in a build() method. Builder methods(Or any initialization methods like constructors) should only have simple logics like input validation or build a field from other fields at most, a complex logic like this should probably be handled before passing values to build().
- If there is no way, please consider add unit tests to cover all the cases. I know it may already be covered by some golden tests, but golden tests are not really unit tests, they are more like a component tests that test multiple classes, which is something we can really improve for this repo in my opinion. Unit test composers/writers may be difficult and don't make sense in some cases, for logic in model classes though, we should be able to unit test them without much difficulties.
There was a problem hiding this comment.
I haven't actually seen these cases come up in the integration tests and the few other googleapis libs I tried, but it does occur in some of the unit tests and could potentially occur. If duplicates are not at least considered, we run into errors with duplicate files.
We can't disambiguate on every possible dimension or names get too long/complex so at some point the samples are the "same" sample with different content. See golden units for examples.
Overload samples will have unique names, but consider this example using different variable names:
Sample 1:
String parent = "parent";
rpcCall(parent);
Sample 2:
String child = "child";
rpcCall(child)
Another possible example is with slight difference in response:
Sample 1:
String resource = "resource";
rpcCall(parent);
Sample 2:
String resource = "resource";
rpcCall(resource).get()
I'm not sure if there's a much better place to put this logic. We should consider duplicates within a gapic class. If I disambiguate duplicates as I collect the samples, I'm just doing the same work over and over again. If I try to disambiguate duplicate samples as I create them, I'm not sure there's a guarantee it will always be a duplicate until they're collected. If I wait to do this until I write the samples, it's just pushing the work down the road.
To separate this from build, I will 1.) move this to a helper method that I call before passing them in to create gapic class. and I will 2.) also write some additional unit tests for this logic. The goal for phase 1 snippet gen is the canonical samples, I can still pivot this overload strategy if it doesn't make sense as we start generating sample files.
There was a problem hiding this comment.
I've broken down all the samples and added additional details at in go/java-sample-gen under 'Current Generated Sample Types'.
I cleaned up the sample names one final time and the main duplicate I'm seeing is the first example above with the overload with different variable names. I've only seen it in the unit tests, haven't seen it in the integration tests. I moved this out of GapicClass and handle before I create GapicClass. As this isn't part of phase 1 and a small edge case, I'm ok leaving as if for now. I added some additional comments in the method to better explain what's happening.
...om/google/api/generator/gapic/composer/samplecode/ServiceClientHeaderSampleComposerTest.java
Show resolved
Hide resolved
src/main/java/com/google/api/generator/gapic/composer/samplecode/SampleComposerUtil.java
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed! |
…lass samples (pt 2) (#970) * chore: rename files * feat: prepare for writing samples, include comment in inline samples * chore: update test naming * test: refactor grpc golden unit tests include samples * test: integration goldens * test: unit goldens * nit refactor * formatting * refactor: move isProtoEmptyType, move handleDuplicateSamples, update sample name/file name * fix - update unit goldens dir with lowercase dir * test: SampleComposerUtilTest included * fix: include header
🤖 I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
go/java-sample-gen