Bug fixes to compile when imported internally. #4499
Conversation
…x the recurring issues seen when we run Copybara
googlebot
added
the
cla: yes
...google-cloud-spanner/src/main/java/com/google/cloud/spanner/v1/stub/SpannerStubSettings.java
Outdated
Show resolved
Hide resolved
| return payload.getDatabasesList() != null | ||
| ? payload.getDatabasesList() | ||
| : ImmutableList.<Database>of(); | ||
| return payload.getDatabasesList(); |
There was a problem hiding this comment.
Does this change behavior?
There was a problem hiding this comment.
No, protobufs are not null and this check is not necessary.
I will need to file a bug for gapic-generators to fix this semantics.
There was a problem hiding this comment.
Ugh. If this is generated code, I'm afraid that we won't be able to check this in. We'd need a generator change instead.
There was a problem hiding this comment.
@sduskis This doesnt change behavior.
@snehashah16 This was done to protected against NPE in paged iteration. Some client like compute do return null instead of empty list. This was done to fix one of such bug #3736
| @@ -72,7 +72,6 @@ public void allOptionsAbsent() { | |||
| assertThat(options.toString()).isEqualTo(""); | |||
| assertThat(options.equals(options)).isTrue(); | |||
| assertThat(options.equals(null)).isFalse(); | |||
| assertThat(options.equals(this)).isFalse(); | |||
There was a problem hiding this comment.
This seems like an interesting test. Why was it removed?
There was a problem hiding this comment.
this is testing 2 different objects( OptionsTest against Options). This may not be interesting because it should always result in false.
There was a problem hiding this comment.
Testing equals against different objects seems like it would exercise a unique codepath, which is "interesting". Shouldn't we keep this test?
|
It looks like your PR introduced issues:
|
sduskis
added
do not merge
JustinBeckwith
added
needs work
|
@snehashah16, I'm closing this issue. It doesn't pass our tests, and there hasn't been any movement for about 2 weeks. |
I will merge this PR when I am ready with verification from the internal tools. Though please provide review comments and|or approval for me to merge.