Add builder methods to EmptyMessage #685
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #685 +/- ##
===========================================
- Coverage 75.63% 75.4% -0.23%
Complexity 1029 1029
===========================================
Files 195 195
Lines 4613 4627 +14
Branches 356 356
===========================================
Hits 3489 3489
- Misses 966 980 +14
Partials 158 158
Continue to review full report at Codecov.
|
|
PTAL |
gax-httpjson/src/main/java/com/google/api/gax/httpjson/EmptyMessage.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public Builder clone() { | ||
| Builder newBuilder = new Builder(); |
There was a problem hiding this comment.
We could just do return new Builder();?
OTOH, is the method really needed though? cloning a Builder object doesn't sound like a valid use case.
There was a problem hiding this comment.
There was a problem hiding this comment.
Because com.google.protobuf.Empty is also a GeneratedMessageV3, I'm pretty sure it would also look like this. (idk how to look at generated protobuf classes tho so i can't confirm)
There was a problem hiding this comment.
IntelliJ usually downloads the sources of the maven artifacts used in project automatically, so you can navigate to the sources just from there. If it can't find sources it will show decompiled version (can be good enough as well). The sources of maven artifacts are usually published together with compiled classes (sometimes in same jar, sometimes as a separate -sources.jar). For the Empty class, its sources can be found here: protobuf-java-3.6.1-sources.jar
There was a problem hiding this comment.
If this method should be there, let's just keep the method and change it to return new Builder(); then :)
| public Builder(EmptyMessage other) {} | ||
|
|
||
| public EmptyMessage build() { | ||
| return new EmptyMessage(); |
There was a problem hiding this comment.
It seems you want to enforce singleton on EmptyMessage, then probably:
- make the constructor private
- change
new EmptyMessage()toDEFAULT_INSTANCE, since we want to make sure there is only one instance of this class - probably make a
DEFAULT_INSTANCEforBuilder. If there will be only one instance ofEmptyMessage, then we probably only need one instance ofBuilder.
There was a problem hiding this comment.
I wanted this to emulate the com.google.protobuf.Empty java class.
There was a problem hiding this comment.
It's not that EmptyMessage should be a singleton, only the DEFAULT_INSTANCE.
There was a problem hiding this comment.
I think it's OK to make EmptyMessage a singleton, because EmptyMessage does not have any fields, and thus all the instances of this class would be the same.
|
I don't fully understand the motivation here, but if you need to make it compatible with other |
|
PTAL |
|
I figure emulating the implementation of the generated class is the safest thing to do, since the protobuf generated message types have gone through a lot of design review. I myself don't know the subtleties of static variable initialization vs initialization in a static block; I'm not sure how synchronization works with singletons, etc. In any case, this would not be an incorrect implementation. |
|
Oh I see. I also don't have enough knowledge to argue against this. I'm summoning @vam-google to TAL. |
|
@hzyi-google @andreamlin As a summoned creature in this PR, I think the most helpful thing I can do here is to provide my view on the PRs motivation and chosen implementation strategy. About using protobuf implementation as a reference. At the same time there are always multiple ways of achieving same result, so the protobuf's way is not the only correct way. We should keep in mind that protobuf classes are generated. Manually written code and generated code have fundamental differences, because generated code is usually machine/automation friendly and manually written code is human/readability friendly, but the both "codes" (automation-friendly and readability-friendly) may be equally correct. What is good for machine-friendliness is usually bad (not always, but often) for readability and vice versa. Here by machine/automation friendliness I mean that for generated classes (their implementation, not the surface) the actual "code" of them is not what is generated, but the templates (or whatever is used), from which it is generated. Generated code for code generators (like protoc or gapic-generator) is like what java bytecode is for java compiler, and templates for the code generators are like what java sources are for java compiler. Why does all this matter? Please see the next paragraph. After the code is created (manually or generated) it must be maintained/modified/improved. For manual case it is all about the code itself, but for the generated case, what should have be maintained is not the generated implementation, but the generator and templates. Using the analogy with java compiler (see above), trying to put generated code in manually written class is like trying to write java directly in bytecode. Yes, I'm absolutely over-exaggerating it here, but there is a portion truth in this exaggeration. Instead of conclusion: there is nothing wrong in copying some parts of protobuf's generated classes, but I don't think we should make manually written classes look identical to them, because it is going to add a lot of overhead without real benefits. We may have to make manual clients identical to protobuf ones (including implementation, not only the surface) in case if they have to be used interchangeably (for example if we need to marshal/unmarshal them by protobuf runtime). Is it the case here? Probably no. Is emulating the surface enough? Static variable initialization vs initialization in a static block private final static String stuff0 = "oops";is effectively same as private final static String stuff0;
static {
stuff0 = "oops"
}Both initializations will happen only once, when the class is first initialized (loded into memory from .class files). But keep in mind that in case of static initializers, the order matters. private static String stuff0 = "oops0";
static {
stuff0 = "oops1";
}the value of But for the following: static {
stuff0 = "oops1";
}
private static String stuff0 = "oops0";the value of But if the variable is assigned only once (and is final) it all does not matter, and static block and static inline initialization should be equal to each other. Synchronization and singletons
The singleton isntance is declared as Misc |
This goes along with [googleapis/gax-java#685](googleapis/gax-java#685), which allows EmptyMessage to be constructed like a proto message. This allows EmptyMessage to have a non-null test value, just like com.google.protobuf.Empty is constructed.
Codecov Report
@@ Coverage Diff @@
## master #685 +/- ##
===========================================
- Coverage 75.63% 75.4% -0.23%
Complexity 1029 1029
===========================================
Files 195 195
Lines 4613 4627 +14
Branches 356 356
===========================================
Hits 3489 3489
- Misses 966 980 +14
Partials 158 158
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #685 +/- ##
============================================
- Coverage 75.63% 75.32% -0.31%
- Complexity 1029 1030 +1
============================================
Files 195 195
Lines 4613 4628 +15
Branches 356 357 +1
============================================
- Hits 3489 3486 -3
- Misses 966 984 +18
Partials 158 158
Continue to review full report at Codecov.
|
|
PTAL |
|
Thanks @hzyi-google and @vam-google ! |
This goes along with [googleapis/gax-java#685](googleapis/gax-java#685), which allows EmptyMessage to be constructed like a proto message. This allows EmptyMessage to have a non-null test value, just like com.google.protobuf.Empty is constructed.
5 participants
So that EmptyMessage has the same calling structures as other ApiMessage implementations, e.g. the Address resource class.