[Storage] Flattening Client Hierarchy - storage-queue #5579
Conversation
HarshaNalluru
requested review from
jeremymeng,
jiacfan,
vinjiang and
XiaoningLiu
as code owners
| this.pipeline.toServiceClientOptions() | ||
| ); | ||
| // Override protocol layer's default content-type | ||
| (storageClientContext as any).requestContentType = undefined; |
There was a problem hiding this comment.
extract common code to a helper that takes url and pipeline then returns a storage client context
Considering a followup PR for updating test titles/test refactoring. Any opinions? |
| */ | ||
| public async enqueueMessage( | ||
| messageText: string, | ||
| options: MessagesEnqueueOptions = {} |
There was a problem hiding this comment.
The name MessagesEnqueueOptions was for the enqueue method on MessagesClient.
Now that it is moved to QueueClient and the method is enqueueMessage, should this be renamed to EnqueueMessageOptions?
MessagesEnqueueOptions -> EnqueueMessageOptions or QueueEnqueueMessageOptions.
Same is applicable for many other options/responses interfaces in this file.
There was a problem hiding this comment.
We will have to rename these for consistency. Let's leave them as is for now.
HarshaNalluru
requested review from
xirzec and
ramya-rao-a
and removed request for
xirzec
|
|
||
| // Override protocol layer's default content-type | ||
| const storageClientContext = this.storageClientContext as any; | ||
| storageClientContext.requestContentType = undefined; |
There was a problem hiding this comment.
Refactoring - Defined getStorageClientContext in utils.common.ts file so that it can be reused for messagesContext and messageIdContext
| public async createQueue( | ||
| queueName: string, | ||
| options: QueueCreateOptions = {} | ||
| ): Promise<Models.QueueCreateResponse> { |
There was a problem hiding this comment.
Shouldn't this have a tracing span for it?
There was a problem hiding this comment.
Logged an issue #5595
| * @returns {Promise<Models.QueueDeleteResponse>} Response data for the Queue delete operation. | ||
| * @memberof QueueServiceClient | ||
| */ | ||
| public async deleteQueue( |
There was a problem hiding this comment.
I thought about it but since this is only a pass-through call it looks okay to not have the span?
There was a problem hiding this comment.
@jeremymeng, @xirzec says that it would be good to have the consistency, If we have spans at both the places, we'll know the actual path it took.
This PR proposes the new shape of QueueClient and QueueServiceClient.
Changes in this PR.
The final version is consistent with the .NET.