-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add TopicName #113
feat: add TopicName #113
Conversation
Codecov Report
@@ Coverage Diff @@
## master #113 +/- ##
==========================================
- Coverage 79.36% 78.37% -1%
+ Complexity 305 302 -3
==========================================
Files 21 21
Lines 2603 2719 +116
Branches 115 134 +19
==========================================
+ Hits 2066 2131 +65
- Misses 467 513 +46
- Partials 70 75 +5
Continue to review full report at Codecov.
|
Hey @chingor13 @kamalaboulhosn could you help with the integration tests? I got:
Also what is the samples test for? |
google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/SubscriptionAdminClient.java
Show resolved
Hide resolved
@@ -360,6 +361,21 @@ public final Subscription createSubscription(Subscription request) { | |||
return createSubscriptionCallable().call(request); | |||
} | |||
|
|||
public final Subscription createSubscription( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation? For this and any of the other public methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this method is only added back for binary backward-compatibility. The functionality of this method is already covered by createSubscription(TopicName topic)
, as the old ProjectTopicName
is the subclass of the new TopicName
.
We are trying to remove the per-pattern resource names (ProjectTopicName
in this case) across all cloud APIs because as we scale it's harder and harder for the generator to come up with meaningful class names. Because PubSub doesn't want a major version bump so we injected code back to the generated clients as a postprocessing step.
So far for such cases we've been only adding essentials (code) back but leaving examples and documentation for all APIs (example). It will be doable to inject the documentation and comments back but will be adding lots of pain for future maintenance. Since I think it'll be PubSub team to continue to maintain the library I want to confirm that you really want me to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should at least have a deprecated notice on this method, indicating the preference for the other createSubscription method and that this one will be removed in the next major version release. I think we do still have to document the public interface as whether or not it is being written manually or not, we need to ensure things are properly documented for our customers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Both deprecation information and documentation is added.
proto-google-cloud-pubsub-v1/src/main/java/com/google/pubsub/v1/RetryPolicyOrBuilder.java
Show resolved
Hide resolved
This PR migrates only synth.py but does not commit the regenerated files. The generation was tested and it works, the updated files are not commited due to breaking changes not related to bazel migration. This PR should be pushed after the an already open PR with the braking changes: googleapis#113
@kamalaboulhosn PTAL |
TopicName
) in this case.TopicName
on gapic surface whereProjectTopicName
was used.synth.py
to add back methods that takeProjectTopicName
.