fix: gax batching regression #863
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## main #863 +/- ##
==========================================
+ Coverage 87.58% 87.60% +0.01%
==========================================
Files 152 152
Lines 15983 15988 +5
Branches 1160 1162 +2
==========================================
+ Hits 13999 14006 +7
- Misses 1637 1641 +4
+ Partials 347 341 -6
Continue to review full report at Codecov.
|
|
Ready for review. |
There was a problem hiding this comment.
LGTM (but please roll back the the ClassNames method access change).
Please consider putting general refactoring chagnes in separate PRs (this one is Ok to keep as is, but for any future changes like this), as most of this PR changes are not related to its description (fixing batch regression) and it makes it hard to reason about and review the actual chagnes.
src/main/java/com/google/api/generator/gapic/composer/Composer.java
Outdated
Show resolved
Hide resolved
...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
Outdated
Show resolved
Hide resolved
| @@ -473,8 +477,7 @@ protected abstract Expr createTransportSettingsInitExpr( | |||
| argList -> | |||
| NewObjectExpr.builder().setType(creatorMethodReturnType).setArguments(argList).build(); | |||
|
|
|||
| TypeNode stubSettingsType = | |||
| typeStore.get(getTransportContext().classNames().getServiceStubSettingsClassName(service)); | |||
| TypeNode stubSettingsType = typeStore.get(ClassNames.getServiceStubSettingsClassName(service)); | |||
There was a problem hiding this comment.
Why is this getting replaced? Is it solely because a static method is accessed via instance reference?
Note, since we already have ClassNames as part of transport context, all the static usages of ClassNames are potentially error-prone (a change in context one will not affect places, which are potentially the subject to change, because they are called statically), so basiclaly we need to convert it the other way around to reach consistency -> convert all Static ClassNames calls to dynamic instance-level ones (and making the method non-static).
The reason why those static methods are still static is historical.
There was a problem hiding this comment.
Is it solely because a static method is accessed via instance reference?
Yes. Code checking tools report that it is simply incorrect and error-prone to access a static method via an instance. (It makes people mistakenly assume that these calls are applying an instance context.)
potentially error-prone
I don't think the change make it error-prone. When it becomes the time you have to make the function non-static, the code will not compile anymore, so you'll have to update the code anyway.
However, I'll revert this change.
There was a problem hiding this comment.
This one's reverted.
| return Optional.of("createPagedCallable"); | ||
| } | ||
|
|
||
| String typeName = callableVarExprType.reference().name(); |
There was a problem hiding this comment.
Minor: why is this style preferable? The older one (if-else) is technically shorter, so seems nicer.
There was a problem hiding this comment.
IMO, the new flow improves readability and reduces cognitive load. If you are familiar with the codebase, it may not matter, but the previous code requires you to read and understand the entire function to know what this is exactly doing. Technically the number of lines is shorter, but it requires you keep following all the contexts, as all the conditional flow must reach the last line. I needed to keep going up and down to make myself certain of the function's behavior. When you immediately return when a certain condition is met, you can stop thinking about the condition at all, reducing the cognitive load to ascertain what is going to happen.
Also, using String.format() makes you to think about it twice every time (sort of over-engineering for a very simple thing), while simply doing return <concrete value> is straight-forward and concisely signals what the function does and returns.
However, if you still prefer the current style, I can happily revert. Please let me know.
There was a problem hiding this comment.
What's your thought?
There was a problem hiding this comment.
Ok. I'm fine with either one, just wanted to make sure that the changes like this are done for a reason (otherwise, convergint one style to another without real purpose would be not a good way of spending time).
🤖 I have created a release *beep* *boop* --- ## [2.8.2](googleapis/java-core@v2.8.1...v2.8.2) (2022-07-13) ### Bug Fixes * enable longpaths support for windows test ([#1485](https://github.com/googleapis/java-core/issues/1485)) ([#866](googleapis/java-core#866)) ([8a8ac99](googleapis/java-core@8a8ac99)) ### Dependencies * update dependency com.google.api-client:google-api-client-bom to v1.35.2 ([#859](googleapis/java-core#859)) ([6b51a1c](googleapis/java-core@6b51a1c)) * update dependency com.google.api:gax-bom to v2.18.3 ([#860](googleapis/java-core#860)) ([f5a5278](googleapis/java-core@f5a5278)) * update dependency com.google.api.grpc:proto-google-common-protos to v2.9.1 ([#855](googleapis/java-core#855)) ([4ec6635](googleapis/java-core@4ec6635)) * update dependency com.google.api.grpc:proto-google-iam-v1 to v1.5.0 ([#862](googleapis/java-core#862)) ([19aebbe](googleapis/java-core@19aebbe)) * update dependency com.google.http-client:google-http-client-bom to v1.42.1 ([#861](googleapis/java-core#861)) ([4d7548b](googleapis/java-core@4d7548b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Fixes #853.