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

[Cognitive Services] Review codesnippets in README #4289

Closed
ramya-rao-a opened this issue Jul 12, 2019 · 7 comments
Closed

[Cognitive Services] Review codesnippets in README #4289

ramya-rao-a opened this issue Jul 12, 2019 · 7 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Mgmt
Milestone

Comments

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Jul 12, 2019

The code-snippets in README for libraries that are auto-generated have also been auto-generated.
This issue is to review these code-snippets and to ensure that they are accurate among the data plane libraries for Cognitive services

Related to #4291

@ramya-rao-a ramya-rao-a added Client This issue points to a problem in the data-plane of the library. Cognitive Services labels Jul 12, 2019
@ramya-rao-a ramya-rao-a added this to the Sprint 156 milestone Jul 12, 2019
@ramya-rao-a ramya-rao-a removed the Epic label Jul 12, 2019
@ramya-rao-a
Copy link
Contributor Author

ramya-rao-a commented Aug 10, 2019

Corrections to the client constructor in the samples:

  • Need to investigate if CognitiveServicesCredentials should be the creds to use in samples or the interactive login.
  • Second argument in constructor should be endpoint, not subscriptionId for the below cases
    • anomalydetector
    • computervision
    • face
    • formRecognizer
    • luis-runtime
    • personalizer
    • textanalytics
  • Second argument in constructor should be options that take the end point -> Service teams to confirm that the code works when no endpoint is provided. If end point is needed, then the signature in the previous point should be used.
    • autosuggest
    • customimagesearch
    • custom search
    • entitysearch
    • image search
    • newssearch
    • spellcheck
    • videosearch
    • visualsearch
    • websearch
  • Second argument in constructor options that takes the base uri -> Service team to check why we use base uri and not endpoint here like most of the other services
    • localsearch
  • Constructor takes apikey and no credentials -> Log issue to check why
    • customvision-prediction
    • customvision-training
  • First argument to constructor is endpoint, second is creds. The order here is flipped when compared to most of the other libraries -> Log issue to check why
    • contentmoderator
    • luis-authoring

Compile errors in typescript sample code

  • Arguments to methods in sample should take options as a json object instead of separate parameters. While this can be fixed by hand for now, the real fix should be in the code generator
    • autosuggest
    • customimagesearch
    • customsearch
    • entitysearch
    • customvision-prediction
    • face
    • imagesearch
    • localsearch
    • luis-authoring
    • luis-runtime
    • newssearch
    • spellcheck
    • textanalytics
    • videosearch
    • visualsearch
  • Remove unused imports
  • anomalydetector, luis-runtime -> use date type not string
  • autosuggest, localsearch - > compile error if responseFormat is specified in options.
  • formrecognizer -> id should be string
  • luis-authoring, luis-runtime -> appId should be string
  • visualsearch -> use of new + require causing trouble, passing image to visualSearch gives errors
  • websearch -> promote in search method causes compile errors

cc @daviwil
fyi @amarzavery

@ramya-rao-a
Copy link
Contributor Author

@daviwil, @amarzavery

Are the below 2 cases a consequence of how the swagger spec was written or the result of using different flags/options when running the code generator? I am trying to see where the long term fix should go.

@amarzavery
Copy link
Contributor

amarzavery commented Aug 12, 2019

In ContentModerator swagger spec the ApiKey is defined in security and securityDefinitions whereas,
in Prediction swagger spec the ApiKey is defined in global parameters section and it has been explicitly stated that x-ms-parameter-location: "client". (More info on "x-ms-parameter-location" extension over here. Adding the extension x-ms-parameter-location: "client" for global parameters is redundant, since that is what Autorest does any ways for global parameters).

Constructor takes apikey and no credentials

Since the generic version of Autorest is used, if you need credentials either use --addcredentials=true or have security defined in the swagger spec.

First argument to constructor is endpoint, second is creds. The order here is flipped

May be they enforced it by adding x-ms-parameter-location: "client" to both the parameters endpoint and apikey and endpoint is defined before apikey in the Prediction swagger spec (It is my theory, not sure about this. Please make changes to the spec and regenerate to verify).

@ramya-rao-a
Copy link
Contributor Author

cc @v-jaswel because he has been involved in getting started scenarios for a couple of Cognitive Services
cc @ChrisHMSFT who can help with reaching out to individual service teams as needed

@ramya-rao-a
Copy link
Contributor Author

Update:
New versions of luis-runtime & luis-authoring libraries have been released with

  • code generated using latest swagger and the latest code generator
  • samples in the readme files fixed.

@sarangan12
Copy link
Member

sarangan12 commented Sep 20, 2019

All Cognitive Services has been released with modified Code Snippets and regenerated code. CLosing this issue as there is no pending action item

@ramya-rao-a
Copy link
Contributor Author

Thanks @sarangan12!
This was a tall order and I am glad that all the cognitive services are up to date with working samples now :)

cc @ChrisHMSFT, @loarabia

@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 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. Cognitive - Mgmt
Projects
None yet
Development

No branches or pull requests

5 participants