-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Code changes to add muli api support #14706
Conversation
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.
@@ -1596,7 +1600,7 @@ export interface SearchIndex { | |||
|
|||
// @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! 👍
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!
* Code changes to add muli api support * Format * Add Version Check * Format * Move ApiVersion to options bag
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