feat: add FunctionalInterface annotation
#1515
Conversation
| @@ -34,6 +34,7 @@ | |||
|
|
|||
| /** Interface for functionality to enhance headers for an http-json call. */ | |||
| @BetaApi | |||
| @FunctionalInterface | |||
There was a problem hiding this comment.
I would not add FunctionalInterface to anything that is marked as BetaApi. Especially for things in httpjson folder.
There was a problem hiding this comment.
Is this because BetaApi is not stable?
| @@ -94,6 +94,7 @@ public Iterator<CollectionT> iterator() { | |||
| }; | |||
| } | |||
|
|
|||
| @FunctionalInterface | |||
There was a problem hiding this comment.
I don't think adding FunctionalInterface to a private interface has value, as we are free to change them at any time.
| @@ -51,6 +51,7 @@ | |||
| */ | |||
| @BetaApi | |||
| public class MtlsProvider { | |||
| @FunctionalInterface | |||
There was a problem hiding this comment.
Same comment as the above private one, I don't think adding FunctionalInterface to a non-public interface has value, this interface should probably be marked as private in the first place also.
There was a problem hiding this comment.
Removed the annotation.
I'll leave the interface as public since changing it to private is out of scope of this PR.
| @@ -39,6 +39,7 @@ | |||
| */ | |||
| public class ValidationException extends IllegalArgumentException { | |||
|
|
|||
| @FunctionalInterface | |||
There was a problem hiding this comment.
This and above inner-public interface are so weird, probably because we wanted to mimic the same behavior of java.util.function.Supplier in Java 7. That being said, I think adding FunctionalInterface to them are appropriate because they are exposed on the surface through other public methods. But we may want to consider replacing these Supplier with java.util.function.Supplier as part of the Java 8 adoption project. CC: @suztomo
There was a problem hiding this comment.
Thanks for the comments.
I'll do a research on replacing Supplier with java.util.function.Supplier. Leave it as-is as this is out of scope of this PR.
There was a problem hiding this comment.
It would break binary compatibility, and so we'd need a major rev bump.
There was a problem hiding this comment.
It would break binary compatibility, and so we'd need a major rev bump.
Are you referring to the idea of replacing Supplier with java.util.function.Supplier? I think it depends on how we approach it, if we do it naively yes it would be a breaking change. We could approach it in steps like adding alternatives first, replacing the ones that are not on the surface first etc.
| @@ -35,6 +35,7 @@ | |||
| /** Provider of custom REST ClientInterceptors. */ | |||
| @BetaApi( | |||
| "The surface for adding custom interceptors is not stable yet and may change in the future.") | |||
| @FunctionalInterface | |||
There was a problem hiding this comment.
Can you remove this as well since it's beta?
| @@ -29,6 +29,7 @@ | |||
| */ | |||
| package com.google.api.gax.batching; | |||
|
|
|||
| @FunctionalInterface | |||
| public interface BatchMerger<B> { | |||
| void merge(B batch, B newBatch); | |||
There was a problem hiding this comment.
In future, if we would add void merge(List<B>);, then this shouldn't be a functional interface.
There was a problem hiding this comment.
The interface is called BatchMerger, it's weird to merge a list.
|
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed! |
|
[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed! |
| } | ||
| }, | ||
| context); | ||
| internalCall(bidiObserver, bidiObserver, context); |
|
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed! |
|
Let's wait to schedule changes together. Also get a review from Ben, who requested this. |
suztomo
added
the
do not merge
suztomo
removed
the
do not merge
Fixes #1516 ☕️