Skip to content
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

Change the messageText and messageId order for updateMessage #4458

Closed
sima-zhu opened this issue Jul 17, 2019 · 6 comments
Closed

Change the messageText and messageId order for updateMessage #4458

sima-zhu opened this issue Jul 17, 2019 · 6 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)

Comments

@sima-zhu
Copy link
Contributor

Proposed order: messageId, popReceipt, messageText, visibilityTimeout
See what the other languages are doing, if it is consistent talk with them and discussing this change

@sima-zhu sima-zhu added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Jul 17, 2019
@sima-zhu sima-zhu self-assigned this Jul 17, 2019
@tg-msft
Copy link
Member

tg-msft commented Aug 12, 2019

In .NET we have

public virtual async Task<Response<UpdatedMessage>> UpdateMessageAsync(
    string messageText,
    string messageId,
    string popReceipt,
    TimeSpan visibilityTimeout = default,
    CancellationToken cancellationToken = default)

which puts messageText first to match EnqueueMessage.

I have no problem making this change if other languages would like to.

@rakshith91
Copy link

In Python, we have
def update_message(self, message, visibility_timeout=None, pop_receipt=None, content=None, timeout=None, **kwargs):
It can also be used to update the contents of the message.

@HarshaNalluru
Copy link
Member

HarshaNalluru commented Aug 12, 2019

JS version - similar to python

// * @memberof MessageIdClient

  public async update(
    popReceipt: string,
    message: string,
    visibilityTimeout?: number,
    options: MessageIdUpdateOptions = {}
  ): Promise<Models.MessageIdUpdateResponse> 

@kurtzeborn
Copy link
Member

XStore team agrees that we should have the same order of parameters on these. The slight preference is towards this ordering: messageText, popReceipt, visibilityTimeout, options. This seems to order them by most used to least used.

@JonathanGiles
Copy link
Member

When I raised this with Sima, it was related to my preference that method arguments flow from arguments that identify the object to perform the operation on, and then to follow that with the values to provide to the object. In this way, it reads out as "update messageID / popReceipt to have a messageText as specified". Currently it reads "update to messageText on messageID / popReceipt", which feels back to front. I have a very weak opinion on this though.

@alzimmermsft
Copy link
Member

Closing this as Java is consistent with .NET

@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

No branches or pull requests

7 participants