feat(topic): create setMetadata method #537
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #537 +/- ##
==========================================
+ Coverage 94.78% 94.81% +0.03%
==========================================
Files 16 16
Lines 959 965 +6
Branches 84 86 +2
==========================================
+ Hits 909 915 +6
Misses 42 42
Partials 8 8
Continue to review full report at Codecov.
|
| type MetadataCallback = RequestCallback<TopicMetadata>; | ||
| type MetadataResponse = [TopicMetadata]; | ||
|
|
||
| export type GetTopicMetadataCallback = MetadataCallback; |
There was a problem hiding this comment.
So - why not just use MetadataCallback over creating all of these specialized interfaces?
There was a problem hiding this comment.
At this point it would be another breaking change, I thought maybe I'd give ya'll a break from those :p
There was a problem hiding this comment.
It's my first week so please put up with me 😆
If I'm reading #496 correctly, the issue is that testing environments are populating the labels field on a topic, and you want to use the method setMetadata to pass {labels: {}} and zero out this value? this seems reasonable to me.
I'm wondering if it might be wroth calling the method updateTopic, so that it better matches the naming conventions of the docs; especially since, if I'm reading correctly, this method could also be used to change the topic name?
| @@ -440,7 +448,7 @@ export class Topic { | |||
| topic: this.name, | |||
| }; | |||
|
|
|||
| this.request<google.pubsub.v1.ITopic>( | |||
There was a problem hiding this comment.
I like that google.pubsub.v1.ITopic has been abstracted into a variable earlier in the file, reads better.
| /** | ||
| * Updates the topic. | ||
| * | ||
| * @see [UpdateTopicRequest API Documentation]{@link https://cloud.google.com/pubsub/docs/reference/rest/v1/UpdateTopicRequest} |
There was a problem hiding this comment.
from reading this documentation, am I reading correctly that labels are the only "meta information" you would potentially change on a topic, and this method since it's calling updateTopic would also potentially be able to change the name of a topic?
I wonder if a better method name would just be updateTopic? which would better match the upstream API too?
There was a problem hiding this comment.
setMetadata is the generic method name we use across all the libraries to update resource fields. I'm definitely not married to it or anything, just went with it for the sake of consistency. I'd be ok with moving away from some of our older conventions to better mirror the upstream apis.
@JustinBeckwith @stephenplusplus @jkwlui what do you guys think?
|
Why isn't Topic a ServiceObject? |
|
So I had that question at first too. Then I noticed - this module doesn't use any of the |
|
@stephenplusplus My memory is kind of fuzzy on this, but I think when we switched from the rest api to grpc the direction was to just use gax and skip all the common lib stuff. |
Relates to #496