[Azure Search] SDK for Autocomplete by mhko · Pull Request #4338 · Azure/azure-sdk-for-net

mhko · 2018-05-18T01:02:16Z

While the swagger spec for autocomplete is being reviewed (https://github.com/Azure/azure-rest-api-specs-pr/pull/473). I wanted to share this PR with you for early visibility. Once the PR on the swagger spec is reviewed and merged, I will regenerate this code from the spec the right way and update this PR.

Description

This checklist is used to make sure that common guidelines for a pull request are followed.

Please add REST spec PR link to the SDK PR
I have read the contribution guidelines.
The pull request does not introduce breaking changes.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

SDK Generation Guidelines

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

…ew SDK

…te API

The Autocomplete tests needed to be re-recorded since they were orginally recorded for an older api-version (2016-09-01-Preview). SearchTests needed to be re-recorded because the test data changed as part of implementing the Autocomplete tests.

dsgouda · 2018-05-18T18:14:02Z

@mhko Please fix test failures

brjohnstmsft · 2018-05-18T19:08:34Z

@mhko It looks like you forgot to add the new SessionRecords that you recorded for the Autocomplete tests. Always make sure that tests pass in Playback mode before submitting a PR.

brjohnstmsft · 2018-05-18T19:09:34Z

@mhko Please always include [Azure Search] in your commit messages

brjohnstmsft · 2018-05-18T19:12:10Z

...SDKs/Search/DataPlane/Microsoft.Azure.Search.Data/Generated/Models/AutocompleteParameters.cs

+        /// highlighting is disabled.</param>
        /// <param name="minimumCoverage">A number between 0 and 100 indicating
-        /// the percentage of the index that must be covered by a suggestion
+        /// the percentage of the index that must be covered by am autocomplete


am autocomplete -> an autocomplete

Fixed in the swagger spec.

brjohnstmsft · 2018-05-18T19:13:11Z

...SDKs/Search/DataPlane/Microsoft.Azure.Search.Data/Generated/Models/AutocompleteParameters.cs

        /// <summary>
        /// Gets or sets the comma-separated list of field names to consider
-        /// when querying for suggestions.
+        /// when querying for autucompleted terms.


autucompleted -> auto-completed

Thanks. Fixed in the swagger spec.

brjohnstmsft · 2018-05-18T19:14:30Z

...Search/DataPlane/Microsoft.Azure.Search.Data/Customizations/Documents/DocumentsOperations.cs

            string suggesterName,
            SearchRequestOptions searchRequestOptions = default(SearchRequestOptions),
-            AutocompleteParametersPayload autocompleteParametersPayload = null,
+            AutocompleteParameters autocompleteParametersPayload = null,


The parameter shouldn't have "payload" in the name

…s PR comments.

brjohnstmsft · 2018-05-18T22:28:29Z

...Search/DataPlane/Microsoft.Azure.Search.Data/Customizations/Documents/DocumentsOperations.cs

            else
            {
+                string searchFieldsStr = null;
+                if (autocompleteParameters != null && autocompleteParameters.SearchFields != null)


nit: You can shorten this to if (autocompleteParameters?.SearchFields != null)

brjohnstmsft · 2018-05-18T22:30:15Z

src/SDKs/Search/DataPlane/Microsoft.Azure.Search.Data/Generated/DocumentsProxyOperations.cs

            if (searchFields != null)
            {
-                _queryParameters.Add(string.Format("searchFields={0}", System.Uri.EscapeDataString(searchFields)));
+                _queryParameters.Add(string.Format("searchFields={0}", System.Uri.EscapeDataString(string.Join(",", searchFields))));


Wow, AutoRest understands comma-separated parameters? That's pretty cool!

brjohnstmsft · 2018-05-18T22:32:17Z

...ionRecords/Microsoft.Azure.Search.Tests.GetAutocompleteTests/TestAutocompleteGetOneTerm.json

+  "Entries": [
+    {
+      "RequestUri": "/subscriptions/3c729b2a-4f86-4bb2-abe8-4b8647af156c/resourceGroups/azsmnet4511/providers/Microsoft.Search/searchServices/azs-7938?api-version=2015-08-19",
+      "EncodedRequestUri": "L3N1YnNjcmlwdGlvbnMvM2M3MjliMmEtNGY4Ni00YmIyLWFiZTgtNGI4NjQ3YWYxNTZjL3Jlc291cmNlR3JvdXBzL2F6c21uZXQ0NTExL3Byb3ZpZGVycy9NaWNyb3NvZnQuU2VhcmNoL3NlYXJjaFNlcnZpY2VzL2F6cy03OTM4P2FwaS12ZXJzaW9uPTIwMTUtMDgtMTk=",


This looks very wrong. There's only a call to ARM to delete the test search service.

Thanks for noticing it. It looks like this session was recorded while refactoring was in progress. This test doesn't exist anymore.

brjohnstmsft · 2018-05-18T22:33:21Z

src/SDKs/Search/DataPlane/Search.Tests/Tests/AutocompleteTests.cs

@@ -172,7 +172,7 @@ protected void TestAutocompleteWithSelectedFields()
            SearchIndexClient client = GetClientForQuery();
            var autocompleteParameters = new AutocompleteParameters()
            {
-                SearchFields = "hotelName"
+                SearchFields = new[] { "hotelName" }


We need tests with more than one search field.

Added one more

autocomplete.

brjohnstmsft · 2018-05-21T16:11:05Z

@mhko This looks good. I'm going to go ahead and merge it, and you can submit another PR once the Swagger PR is merged and you're able to regenerate the code from the spec in Azure/master.

brjohnstmsft and others added 4 commits May 11, 2018 12:10

[Azure Search] Applying Autocomplete test changes to 2017-11-11-Previ…

a1058a3

…ew SDK

[Azure Search] Regenerating from new Swagger that includes Autocomple…

1ee696f

…te API

Regenerated code for Autocomplete. Refactored tests.

fd4a388

dsgouda added the Changes Requested label May 18, 2018

brjohnstmsft suggested changes May 18, 2018

View reviewed changes

[Azure Search] Updating SessionRecords for autocomplete tests. Addres…

12d3573

…s PR comments.

brjohnstmsft suggested changes May 18, 2018

View reviewed changes

[Azure Search] Adding two tests. Clean up and re-recording of tests for

57ba2d5

autocomplete.

brjohnstmsft approved these changes May 19, 2018

View reviewed changes

brjohnstmsft merged commit b3ba966 into Azure:search-preview May 21, 2018

Conversation

mhko commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

dsgouda commented May 18, 2018

Uh oh!

brjohnstmsft commented May 18, 2018

Uh oh!

brjohnstmsft commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brjohnstmsft commented May 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mhko commented May 18, 2018 •

edited

Loading