-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Regenerated Cognitive Services SDK Face API #14240
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
Conversation
| * Similar](https://docs.microsoft.com/rest/api/cognitiveservices/face/face/findsimilar). | ||
| * <br /> After creation, user should use [FaceList - Add | ||
| * Face](https://docs.microsoft.com/rest/api/cognitiveservices/face/facelist/addfacefromurl) to | ||
| * Similar](../face/findsimilar). |
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.
Do you know why the links are changing here? How does this relative link help in editors? Or in docs.microsoft.com for that matter
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.
@ramya-rao-a Let me explain with an example. Today you can visit https://docs.microsoft.com/en-us/javascript/api/@azure/cognitiveservices-face/facelistoperations?view=azure-node-latest. In create you have a link https://docs.microsoft.com/en-us/rest/api/cognitiveservices/face/face/findsimilar which leads you to 404 Page not found error.
Now, they have modified to ../face/findsimilar (similar pattern could be found all through the file) I am able to confirm the link works fine upto ../face. The second part is not working now. But, my assumption is that since this is a conscious change they should have worked out with the docs team to get it right. But as of now, it works only upto ../face.
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.
Adding @zhizhoualan @lmazuel
@zhizhoualan Could you please confirm if the relative links will actually work after the release. As of today, if I manually try these types of links, they are failing in docs.microsoft.com
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.
Thanks @sarangan12
We have similar requests for other languages as well. Can you co-ordinate with them and let them know about this issue which will affect them as well? You can use issues created for other languages just like https://github.com/Azure/sdk-release-request/issues/1333 was done for JS
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.
@sarangan12 the relative path change has been published to docs.microsoft.com, they are working in there. I see the link you tested is incorrect at "cognitiveservice" folder. the correct link is here: https://docs.microsoft.com/en-us/rest/api/faceapi/face/findsimilar, you see doc team updated cognitiveservice to faceapi, I also don't understand the reason, but Nitin suggest us change to relative path so that we get away from such impact.
but here do we want to make sure the comments in .ts file also works? if so that we have to change them back to absolute path, which will be error prone for base path update we well.
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.
Most of relative paths worked. But we do find some broken links. I will update these links and send new request for release. Thanks.
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 just merged our latest PR using absolute links (Azure/azure-rest-api-specs#13394). Could you please make the release based on the latest version? Thanks. @sarangan12 @ramya-rao-a
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.
I have updated the PR with SDK generated from the latest commit. Command Used:
C:\Users\sarajama\Projects\azure-sdk-for-js>autorest --typescript --license-header=MICROSOFT_MIT_NO_VERSION --typescript-sdks-folder=. [email protected]/[email protected] --package-version=4.2.0 https://raw.githubusercontent.com/Azure/azure-rest-api-specs/b81a30cfeca9873872374279e8351923326cdb04/specification/cognitiveservices/data-plane/Face/readme.md
AutoRest code generation utility [cli version: 3.0.6339; node: v14.15.4, max-memory: 4096 MB]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
There is a new version of AutoRest available (3.1.2).
> You can install the newer version with with npm install -g autorest@latest
NOTE: AutoRest core version selected from configuration: ~2.0.4413.
Loading AutoRest core 'C:\Users\sarajama\.autorest\@[email protected]\node_modules\@microsoft.azure\autorest-core\dist' (2.0.4417)
Loading AutoRest extension '@microsoft.azure/autorest.typescript' (4.2.2->4.2.2)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.51->2.3.51)
@ramya-rao-a Kindly review and approve the PR.
This is to addess the Issue: https://github.com/Azure/sdk-release-request/issues/1333
Changes Included:
Regenerated SDK using command:
I don't see any breaking change. So, I have updated the minor version.
Note: I have used the same autorest.typescript version as used during the last release.
@ramya-rao-a Please review and approve