fix(types): Use fully-qualified message type names [ggj] #723
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #723 +/- ##
==========================================
- Coverage 92.52% 92.33% -0.20%
==========================================
Files 124 124
Lines 13459 13489 +30
Branches 981 994 +13
==========================================
+ Hits 12453 12455 +2
- Misses 765 784 +19
- Partials 241 250 +9
Continue to review full report at Codecov.
|
| @@ -30,6 +30,9 @@ | |||
| public abstract class Message { | |||
| public abstract String name(); | |||
|
|
|||
| // The fully-qualified proto name, which differs from the Java fully-qualified name. | |||
| public abstract String fullProtoName(); | |||
There was a problem hiding this comment.
Can you please specify how exactly they are different? Is it about the extra com. prefix for java names, or more than that?
There was a problem hiding this comment.
Yes, it's the proto package versus the java_package. Added an example.
| // These can be short names (e.g. FooMessage) or fully-qualified names with the *proto* package. | ||
| String responseTypeName = lroInfo.getResponseType(); | ||
| String metadataTypeName = lroInfo.getMetadataType(); | ||
| String[] responseTypeNameComponents = responseTypeName.split("\\."); |
There was a problem hiding this comment.
Minor, but it seems like you don't need the full set of compontents, only the last element after dot, so it can be something like.
String responseTypeShortName = responseTypeName.substring(responseTypeName.lastIndexOf(".") + 1)
It is not about performance, mainly just to make it clear that the other compnents are not needed to the readers of the code.
There was a problem hiding this comment.
Done. FWIW, the tradeoff here is an extra check that the index is >= 0.
| // The messageTypes map keys to the Java fully-qualified name. | ||
| for (Map.Entry<String, Message> messageEntry : messageTypes.entrySet()) { | ||
| String[] packageComponents = messageEntry.getKey().split("\\."); | ||
| String messageShortName = packageComponents[packageComponents.length - 1]; |
There was a problem hiding this comment.
Similar situation as with responseTypeShortName and metadataTypeShortName
| for (Map.Entry<String, Message> messageEntry : messageTypes.entrySet()) { | ||
| String[] packageComponents = messageEntry.getKey().split("\\."); | ||
| String messageShortName = packageComponents[packageComponents.length - 1]; | ||
| if (responseMessage == null) { |
There was a problem hiding this comment.
The responseMessage logic and metadataMessage logic seem to be a copypaste of each other. Please consider moving that into a private or method combining this to a for loop with 2 elements in it (response and metadata messages).
There was a problem hiding this comment.
I'd considered using these, which would result in the following tradeoffs:
- Nested loop: Need an extra map or data structure to associate
responseMessage,responseTypeName,isResponseTypeNameShortOnly, which would be less readable, or recompute the latter in the loop (which is inefficient). - Separate method: Need to take four parameters at best - the three above plus
messageEntry, which would make the call site, helper signature, and method itself less readable.
Given that the logic is relatively small, copy-pasting does not seem to have significantly better or worse tradeoffs compared to the alternative approches.
* chore: update dependencies for regapic * add more dependencies and trigger comment * update goldens * fix indentation * remove duplicate gax-httpjson dependency * remove duplicated dependencies Source-Link: googleapis/synthtool@fa54eb2 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:1ec28a46062b19135b11178ceee60231e5f5a92dab454e23ae0aab72cd875906
…-plugin to v3.3.2 (#723) [](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [org.apache.maven.plugins:maven-javadoc-plugin](https://maven.apache.org/plugins/) | `3.3.1` -> `3.3.2` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-core).
](https://renovatebot.com){kind=link}
Addresses the use case where a proto message
foo.bar.Carhas an internal field with the same short name, such asbar.foo.Car.