Conversation
gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java
Outdated
Show resolved
Hide resolved
gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
Outdated
Show resolved
Hide resolved
gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
Outdated
Show resolved
Hide resolved
…atches proto package in parser. other cleanups and minor fixes.
gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
Outdated
Show resolved
Hide resolved
| serviceDescriptor -> { | ||
| List<MethodDescriptor> methodsList = serviceDescriptor.getMethods(); | ||
| if (methodsList.isEmpty()) { | ||
| List<MethodDescriptor> methodListSelected = |
There was a problem hiding this comment.
This scenario is in case that all methods from a service are excluded?
There was a problem hiding this comment.
Yes. And this is likely now e.g. in vertexai's requirement it only exposes methods from 2 services.
gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private static Optional<List<String>> getInclusionMethodListFromServiceYaml( | ||
| Optional<com.google.api.Service> serviceYamlProtoOpt, String protoPackage) { |
There was a problem hiding this comment.
It is usually preferred to not passing around Optional if we can check it before calling the method.
There was a problem hiding this comment.
I want to follow-up on this from our discussion earlier and get @blakeli0 your opinion on this. (FYI, I've combined this method into shouldIncludeMethodInGeneration() )
For now, I am slightly inclined to keep this Optional<com.google.api.Service> serviceYamlProtoOpt as input for 2 reasons:
- The pattern of passing in this
Optionalis repeated in thisParserclass, a larger scale refactor is needed to remove these. - Because this service yaml is an optional config file that only some libraries have, it sort of make sense to have this Optional representation. And I find the current way can better encapsulate this piece of logic:
shouldIncludeMethodInGeneration()has all the information to decide if this method should or not be included. I could checkserviceYamlProtoOptbefore calling the method, but the 2 occurrences would repeat the logic in a reverse way, and it might be less readable for debugging in the future.
Does this makes sense to you? WDYT?
gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| void selectiveGenerationTest_shouldExcludeUnusedResourceNames() { |
There was a problem hiding this comment.
This test verifies that unused resource names (associated with methods not in inclusion list) will not be populated to GapicContext.
gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
Outdated
Show resolved
Hide resolved
gapic-generator-java/src/main/java/com/google/api/generator/gapic/protoparser/Parser.java
Outdated
Show resolved
Hide resolved
| Transport.GRPC); | ||
| } | ||
|
|
||
| private static boolean shouldIncludeMethodInGeneration( |
There was a problem hiding this comment.
Can we make this a default scope method so we can test it directly? This would enabled us testing more scenarios like the non matching packages, it would also simplify the golden tests.
gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/ComposerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
We probably don't need this yaml and selective_api_generation_no_publishing_v1beta1.yaml as the scenarios should be tested in ParserTest directly.
#3323) following changes in client.proto that propagated to `ClientProto.java` ([pr](https://github.com/googleapis/sdk-platform-java/pull/3309/files#diff-44c330ef5cfa380744be6c58a14aa543edceb6043d5b17da7b32bb728ef5d85f)), apply changes from poc pr (#3129). For context: [go/selective-api-gen-java-one-pager](http://goto.google.com/selective-api-gen-java-one-pager), b/356380016
|
implemented in #3323. closing |
DO NOT MERGE.
This is a draft pr as prototype to changes for selective api generation feature.
The changes in this pr include api-common-java changes copied over manually for testing purposes.