-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Code changes to add muli api support #14706
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
Code changes to add muli api support #14706
Conversation
xirzec
left a comment
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.
Sorry if I'm not understanding the context behind these changes, but I have some questions.
| // @public | ||
| export class SearchIndexClient { | ||
| constructor(endpoint: string, credential: KeyCredential, options?: SearchIndexClientOptions); | ||
| constructor(endpoint: string, credential: KeyCredential, apiVersion?: string, options?: SearchIndexClientOptions); |
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 don't like having multiple optional parameters. This forces anyone who wants to pass options to pass undefined to apiVersion if they don't want to set it, which feels bad.
Why can't apiVersion live on SearchIndexClientOptions ?
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.
Agreed. I will change it for all the 3 clients in the next commit.
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.
@xirzec Updated in the latest commit
| /** Identifies the concrete type of the normalizer. */ | ||
| odatatype: string; | ||
| /** Polymorphic discriminator, which specifies the different types this object can be */ | ||
| odatatype: "#Microsoft.Azure.Search.CustomNormalizer"; |
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'm confused about this. Why would a generic LexicalNormalizer have the same odatatype as a CustomNormalizer?
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.
@xirzec Yeah. It was little confusing to me too. Let me explain with an example. For the following swagger:
"Similarity": {
"discriminator": "@odata.type",
.........
}
"ClassicSimilarity": {
"x-ms-discriminator-value": "#Microsoft.Azure.Search.ClassicSimilarity",
.........
}
"BM25Similarity": {
"x-ms-discriminator-value": "#Microsoft.Azure.Search.BM25Similarity",
.........
}For the above swagger, the SDK will be generated as:
export interface Similarity {
odatatype: "#Microsoft.Azure.Search.ClassicSimilarity" | "#Microsoft.Azure.Search.BM25Similarity";
.........
}
export type ClassicSimilarity = Similarity & {
odatatype: "#Microsoft.Azure.Search.ClassicSimilarity";
.........
};
export type BM25Similarity = Similarity & {
odatatype: "#Microsoft.Azure.Search.BM25Similarity";
.........
}In the above code, look at the Similarity interface. It is the base class. But, the odatatype of Similarity is actually the union of odatatype of the two sub classes (ClassicSimilarity & BM25Similarity).
Now, let us look into the LexicalNormalizer. The swagger is:
"LexicalNormalizer": {
"discriminator": "@odata.type",
......
}
"CustomNormalizer": {
"x-ms-discriminator-value": "#Microsoft.Azure.Search.CustomNormalizer",
..........
}For the above swagger, the SDK looks like:
export interface LexicalNormalizer {
odatatype: "#Microsoft.Azure.Search.CustomNormalizer";
.........
}
export type CustomNormalizer = LexicalNormalizer & {
odatatype: "#Microsoft.Azure.Search.CustomNormalizer";
.........
}In the above code the base class LexicalNormalizer, the odatatype is set to that of the CustomNormalizer which may look a little odd.
But, consider there is another normalizer in the swagger such as this:
"SampleNormalizer": {
"x-ms-discriminator-value": "#Microsoft.Azure.Search.SampleNormalizer",
..........
}then the resulting SDK will look like:
export interface LexicalNormalizer {
odatatype: "#Microsoft.Azure.Search.CustomNormalizer" | "#Microsoft.Azure.Search.SampleNormalizer";
.........
}
export type CustomNormalizer = LexicalNormalizer & {
odatatype: "#Microsoft.Azure.Search.CustomNormalizer";
.........
}
export type SampleNormalizer = LexicalNormalizer & {
odatatype: "#Microsoft.Azure.Search.SampleNormalizer";
.........
}The confusion is that we do not have more than one sub type of normalizer, it is looking a little odd. Am I making sense?
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.
Ohh! This does make sense and helps a lot. Thanks! 👍
xirzec
left a comment
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.
Update looks good!
Two new requests have come for search-documents SDK.
There is a bug in the current swagger. This should be fixed before next release. In the meantime, we need to add a swagger transform and handle the bug. The issue is detailed here
This issue is regarding passing a different version to the search clients constructors. It has already been communicated to the customers that there might be additional fields coming up due to version changes and to the service team that there should not be any compatibility changes. This design will be dicussed in detail before next release. But for this release, we wanted to try this design of multi api support.
@xirzec Please review this PR