Skip to content

[Question Answering] Authoring Client Initialization#25348

Merged
heaths merged 9 commits intoAzure:mainfrom
AhmedLeithy:QA-LLC
Jan 5, 2022
Merged

[Question Answering] Authoring Client Initialization#25348
heaths merged 9 commits intoAzure:mainfrom
AhmedLeithy:QA-LLC

Conversation

@AhmedLeithy
Copy link
Member

@AhmedLeithy AhmedLeithy commented Nov 16, 2021

Hey all,

This PR is for initializing the QuestionAnswering authoring client. I have the following concerns:

  1. The client is generated with the name QuestionAnsweringProjectsClient, with a plural "projects". I think it makes sense to have the plural "projects" in the name of the client, and as such used the same for the namespace. Do we want the namespace to be ...QuestionAnswering.Project instead?

  2. Using the namespace parameter in autorest.md does not actually set the namespace of the generated file(s). Not sure if this is a bug or if this some sort of mistake on my part, but I changed the namespace using the [CodeGenType] attribute. I left the syntax that I was using as a comment in autorest.md for demonstration.

  3. I removed the newly created options class as mentioned in the comment that was left in autorest.md. I would also like to note that we can keep the class but make it generate with a different name, QuestionAnsweringProjectsClientOptions for example. This can be done by adding another Title property in the autorest.md file for the generated swagger. This will be useful if we expect the runtime and authoring clients to have different version names and/or be out of sync with versioning.

@azure-sdk
Copy link
Collaborator

API changes have been detected in Azure.AI.Language.QuestionAnswering. You can review API changes here

@heaths
Copy link
Member

heaths commented Nov 16, 2021

  1. Given the plural "Projects" is what the service team used for the operation ID, QuestionAnsweringProjectsClient sounds correct: https://github.com/Azure/azure-rest-api-specs/blob/f3c2b263da9afc93edb71d55d9b08c75de7401d9/specification/cognitiveservices/data-plane/Language/stable/2021-10-01/questionanswering-authoring.json#L33

    As for the subnamespace, I think "Projects" still makes sense given plurality of several other subnamespaces, but defer to @KrzysztofCwalina and @tg-msft, as well as @annatisch and @johanste if we want to have a subnamespace in Python.

  2. Where did you put the namespace declaration in autorest.md? In the batch? This should've worked. Given how many models there are, I'd prefer we don't have to create and maintain a bunch of classes just to move. If anything, a transform to add x-namespace to each model would be concise:

    directive:
    - from: swagger-document
      where: $.definitions.*
      transform: $["x-namespace"] = "Azure.AI.Language.QuestionAnswering.Projects";
  3. Having a single QuestionAnsweringClientOptions makes sense. We've done that elsewhere when a separate class provides literally no value such as extra properties. We can always derive a specific class later if necessary.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, but a couple of open questions we should probably resolve.

@azure-sdk
Copy link
Collaborator

API changes have been detected in Azure.AI.Language.QuestionAnswering. You can review API changes here

@AhmedLeithy
Copy link
Member Author

As per @rokulka 's request and to be consistent with python, I've added transforms to ensure that the recent changes made to the operationIds in the swagger do not affect our SDK. I've also updated autorest.md to reference the latest swagger.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make a tracking issue and reference it in the comment, but otherwise LGTM.

@heaths heaths merged commit eba89cd into Azure:main Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants