-
Notifications
You must be signed in to change notification settings - Fork 5.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
[Azure Search] Add Get Documents API (a.k.a. Lookup) to data plane API spec #5169
Conversation
Note that this API already exists in the .NET SDK, but is currently implemented as custom code. We need to add transform directives for the generated code because otherwise there's no way to pass custom JsonSerializerSettings into the generated code (which is a requirement for our data plane). Also renamed some example files to be more consistent with existing naming conventions.
Automation for azure-sdk-for-jsNothing to generate for azure-sdk-for-js |
Automation for azure-sdk-for-goNothing to generate for azure-sdk-for-go |
Automation for azure-sdk-for-pythonNothing to generate for azure-sdk-for-python |
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
Automation for azure-sdk-for-nodeNothing to generate for azure-sdk-for-node |
Automation for azure-sdk-for-javaNothing to generate for azure-sdk-for-java |
Big thanks to @mhko who did the majority of the work on this. |
Can one of the admins verify this patch? |
@jhendrixMSFT Some context on this change: This is not a new API, it's just that until now we've never generated code for it. This is the first of many steps we're taking to make this particular Swagger spec accurately reflect our data plane API. |
@brjohnstmsft do you do any local C# codegen to verify this looks ok? |
@@ -11,7 +11,7 @@ | |||
"highlightPostTag": "</em>", | |||
"highlightPreTag": "<em>", | |||
"minimumCoverage": 80, | |||
"searchFields": ["title", "description"], | |||
"searchFields": "title,description", |
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.
The spec still has it as an array of string
Lines 207 to 212 in 576cce0
"name":"searchFields", | |
"in": "query", | |
"type": "array", | |
"items": { | |
"type": "string" | |
}, |
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.
@nschonni That's the spec for the GET version of Autocomplete. The example I updated was for POST. AutoRest uses array parameters to represent query string parameters that are comma-separated lists. So although it's a comma-separated list everywhere on the wire, in the spec it's a string for POST and an array of strings for GET.
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.
Gotcha, I guess it's the alias'd property here
Lines 518 to 522 in 576cce0
"properties": { | |
"search": { | |
"type": "string", | |
"description": "The search text on which to base autocomplete results.", | |
"x-ms-client-name": "searchText" |
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.
Almost, it's this one:
@jhendrixMSFT Yes, I'm working on a .NET SDK PR that I will send out once this is merged. I wrote the translation rules in tandem with changing the .NET SDK. |
If you are a MSFT employee you can view your work branch via this link.
Contribution checklist: