[Cognitive Services] Update endpoint URL template for TextAnalytics. #3491
Conversation
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-nodeThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-rubyThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
| "parameters": { | ||
| "Endpoint": { | ||
| "name": "Endpoint", | ||
| "description": "Supported Cognitive Services endpoints (protocol and hostname, for example: https://westus.api.cognitive.microsoft.com).", |
There was a problem hiding this comment.
Please note that the Azure Portal resource "Endpoint" includes the /text/analytics/v2.0. We should make sure the documentation is very clear here, or change it to include the suffix.
There was a problem hiding this comment.
Also, please link to the documentation PR as well.
There was a problem hiding this comment.
Hi @assafi, we will update Azure Portal accordingly when we publish SDK.
As in description, I emphasized protocol and hostname and attached an example. Any suggestion how to make it more clear?
What which document you refer to?
- If you refer to API Reference (https://docs.microsoft.com/en-us/rest/api/cognitiveservices/textanalytics/detect%20language/detect%20language), they are auto generated by this swagger, once this get checked in, the document will be automatically updated.
- If you refer to https://github.com/MicrosoftDocs/azure-docs, I think it should be updated along with SDK PR.
There was a problem hiding this comment.
What do you mean by updating the Azure portal?
I'm referring to the TA documentation from your previous PR, which says:
Replace the location in client.BaseUri to the endpoint you signed up for. You can find the endpoint on Azure Portal resource. The endpoint typically looks like "https://[region].api.cognitive.microsoft.com/text/analytics/v2.0".
If you will be using a similar language to describe this you should note that the endpoint from the Azure portal contains a suffix which you exclude in this PR.
Ideally, we should update the SDK and its documentation at the same time, to avoid recent issues like lagging documentation. Which is why I asked the description of this PR to contain a link to the documentation PR.
There was a problem hiding this comment.
I will change Azure Portal resource page to remove the suffix to keep consistent. The document will also say:
You can find the endpoint on Azure Portal resource. The endpoint typically looks like "https://[region].api.cognitive.microsoft.com/"
Does it make sense?
About document PR, I agree SDK (actually SDK package) and its documentation should be updated at the same time. But this is swagger PR?
After merge swagger PR, I will send C# SDK PR. I prefer send the documentation PR along with C# SDK PR, because at that time I can test the code in the documents. Also the document won't be accidentally published before NuGet (sometimes you say "please hold", but...)
We don't need to worry about the lagging documentation. It is hard to control when documents get updated, but we can control when to publish NuGet.
But I can send a document PR now if it is your concern.
There was a problem hiding this comment.
Regarding the portal update - I don't think we need to change it. Not all users use the SDK, some simply write their own code in whatever language they want and they will need the full URL. It's important not to break existing or future customers just because we changed how a parameter looks in the SDK (because of some other swagger limitation). The solution for this should be better documentation and/or smarter SDK.
Regarding the documentation PR - sure, please attach it to the SDK PR.
There was a problem hiding this comment.
We've agreed to go with baseEndpoint instead of Endpoint at a later update.
|
Hi @annatisch we've completed the internal review |
|
Thanks @yangyuan - were you going to rename Endpoint -> baseEndpoint in this PR? |
|
@annatisch not in this PR. |
Detail background and explainations in here: #3489 Cognitive Services URL template (endpoint)
Please hold for TextAnalytics team's review.
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-versionin the path should match theapi-versionin the spec).Quality of Swagger