code refactoring 4 #11800

xiangyan99 · 2020-06-03T22:57:23Z

…11805) Co-authored-by: Charles Lowell <chlowe@microsoft.com>

* [HDInsight] Fix hdi test failure (#11806) * Initial generation Synapse autorest v5 * Fix empty model generation * Fix Test Failure: Skip 3 test case: test_create_with_adlsgen1, test_create_with_additional_storage, test_oms_on_running_cluster Rename test_http_extend to test_gateway_setting for http settings is replaced with gateway settings Co-authored-by: Laurent Mazuel <laurent.mazuel@gmail.com> Co-authored-by: Zhenyu Zhou <zhezhou@microsoft.com> * disable some by design bandit warnings (#11495) * disable some by design bandit warnings * Packaging update of azure-mgmt-datalake-analytics Co-authored-by: Azure SDK Bot <aspysdk2@microsoft.com> * Increment package version after release of azure_core (#11795) * Use subject claim as home_account_id when no client_info (#11639) * Refactor ClientCertificateCredential to use AadClient (#11719) * Refactor ClientSecretCredential to use AadClient (#11718) * [Cosmos] Fixed incorrect ID type error (#11798) * Fixed incorrect ID type error * Fix pylint * [text analytics] Update readme (#11796) * try something (#11797) * Search refactoring 3 (#11804) * create_or_update takes object * rename is_hidden to hidden * fix types * updates Co-authored-by: aim-for-better <zhenyu.zhou@microsoft.com> Co-authored-by: Laurent Mazuel <laurent.mazuel@gmail.com> Co-authored-by: Zhenyu Zhou <zhezhou@microsoft.com> Co-authored-by: Azure SDK Bot <aspysdk2@microsoft.com> Co-authored-by: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com> Co-authored-by: Charles Lowell <chlowe@microsoft.com> Co-authored-by: annatisch <antisch@microsoft.com> Co-authored-by: iscai-msft <43154838+iscai-msft@users.noreply.github.com> Co-authored-by: Krista Pratico <krpratic@microsoft.com>

heaths

A model still needs renaming (see comments), but I'll sign off to keep you unblocked.

heaths · 2020-06-05T02:55:45Z

...arch/azure-search-documents/azure/search/documents/indexes/_internal/_search_index_client.py

@@ -235,7 +235,7 @@ def create_or_update_index(

    @distributed_trace
    def analyze_text(self, index_name, analyze_request, **kwargs):
-        # type: (str, AnalyzeRequest, **Any) -> AnalyzeResult
+        # type: (str, AnalyzeTextRequest, **Any) -> AnalyzeResult


Should be AnalyzeTextOptions now, or was that going to come in a separate PR? See #11830.

Already fixed. :)

xiangyan99 · 2020-06-05T04:32:40Z

/azp run python - search - ci

xiangyan99 · 2020-06-05T04:36:59Z

/azp run python - search - ci

azure-pipelines · 2020-06-05T04:37:13Z

Azure Pipelines successfully started running 1 pipeline(s).

xiangyan99 · 2020-06-05T04:53:39Z

/azp run python - search - ci

azure-pipelines · 2020-06-05T04:53:52Z

Azure Pipelines successfully started running 1 pipeline(s).

tg-msft

This all looks good with my limited understanding of our Python best practices. 😄

tg-msft · 2020-06-05T17:27:28Z

sdk/search/azure-search-documents/azure/search/documents/_internal/aio/_search_client_async.py

+        include_total_result_count = kwargs.pop("include_total_result_count", None)
+        facets = kwargs.pop("facets", None)
+        filter_arg = kwargs.pop("filter", None)
+        highlight_fields = kwargs.pop("highlight_fields", None)
+        highlight_post_tag = kwargs.pop("highlight_post_tag", None)
+        highlight_pre_tag = kwargs.pop("highlight_pre_tag", None)
+        minimum_coverage = kwargs.pop("minimum_coverage", None)
+        order_by = kwargs.pop("order_by", None)
+        query_type = kwargs.pop("query_type", None)
+        scoring_parameters = kwargs.pop("scoring_parameters", None)
+        scoring_profile = kwargs.pop("scoring_profile", None)
+        search_fields = kwargs.pop("search_fields", None)
+        search_mode = kwargs.pop("search_mode", None)
+        select = kwargs.pop("select", None)
+        skip = kwargs.pop("skip", None)
+        top = kwargs.pop("top", None)
+        query = SearchQuery(
+            search_text=search_text,
+            include_total_result_count=include_total_result_count,
+            facets=facets,
+            filter=filter_arg,
+            highlight_fields=highlight_fields,
+            highlight_post_tag=highlight_post_tag,
+            highlight_pre_tag=highlight_pre_tag,
+            minimum_coverage=minimum_coverage,
+            order_by=order_by,
+            query_type=query_type,
+            scoring_parameters=scoring_parameters,
+            scoring_profile=scoring_profile,
+            search_fields=search_fields,
+            search_mode=search_mode,
+            select=select,
+            skip=skip,
+            top=top
+        )


Nit - Is it worth moving this logic onto the SearchQuery class so you don't have to update both the sync/async clients whenever we add a new parameter? I'm not sure how the Python folks typically handle these situations. (Also, we can just file a work item rather for Preview 5 rather than try to fix it today.)

SearchQuery and AutocompleteQuery, SuggestQuery share one ctor. If we put all of pop operation there, I am afraid it will be messed up.

tg-msft · 2020-06-05T17:34:06Z

sdk/search/azure-search-documents/azure/search/documents/indexes/_internal/_models.py

+    ):
+        super(AnalyzeTextOptions, self).__init__(**kwargs)
+        self.text = kwargs['text']
+        self.analyzer_name = kwargs.get('analyzer_name', None)


Just for my own understanding - why get here vs pop in search when taking apart kwargs?

The difference is pop will remove the argument from kwargs. In init method, kwargs will not be used in anywhere else so there is no difference. But in e.g. search method, if I don't pop them, those arguments will be passed into transport which will cause problems.

tg-msft · 2020-06-05T17:35:06Z

sdk/search/azure-search-documents/samples/async_samples/sample_facet_query_async.py

    async with search_client:
-        results = await search_client.search(query=query)
+        results = await search_client.search(search_text="WiFi", facets=["Category"], top=0)


This looks very nice.

rakshith91 · 2020-06-05T17:37:45Z

sdk/search/azure-search-documents/README.md

+client = SearchIndexClient(endpoint="<service endpoint>"
+                             credential=credential)


Suggested change

client = SearchIndexClient(endpoint="<service endpoint>"

credential=credential)

client = SearchIndexClient(endpoint="<service endpoint>",

credential=credential)

rakshith91 · 2020-06-05T17:39:47Z

sdk/search/azure-search-documents/README.md

@@ -64,18 +65,33 @@ client = SearchClient(endpoint="<service endpoint>",
                      credential=credential)
 ```

-#### Create a SearchServiceClient
+#### Create a SearchIndexClient

 Once you have the values of the Cognitive Search Service [service endpoint](https://docs.microsoft.com/en-us/azure/search/search-create-service-portal#get-a-key-and-url-endpoint)
 and [api key](https://docs.microsoft.com/en-us/azure/search/search-security-api-keys) you can create the Search Service client:


Suggested change

and [api key](https://docs.microsoft.com/en-us/azure/search/search-security-api-keys) you can create the Search Service client:

and [api key](https://docs.microsoft.com/en-us/azure/search/search-security-api-keys) you can create the Search Index client:

rakshith91 · 2020-06-05T17:40:32Z

sdk/search/azure-search-documents/README.md

+#### Create a SearchIndexerClient
+
+Once you have the values of the Cognitive Search Service [service endpoint](https://docs.microsoft.com/en-us/azure/search/search-create-service-portal#get-a-key-and-url-endpoint)
+and [api key](https://docs.microsoft.com/en-us/azure/search/search-security-api-keys) you can create the Search Service client:


Suggested change

and [api key](https://docs.microsoft.com/en-us/azure/search/search-security-api-keys) you can create the Search Service client:

and [api key](https://docs.microsoft.com/en-us/azure/search/search-security-api-keys) you can create the Search Indexer client:

rakshith91 · 2020-06-05T17:40:42Z

sdk/search/azure-search-documents/README.md

+
+credential = AzureKeyCredential("<api key>")
+
+client = SearchIndexerClient(endpoint="<service endpoint>"


Suggested change

client = SearchIndexerClient(endpoint="<service endpoint>"

client = SearchIndexerClient(endpoint="<service endpoint>",

rakshith91 · 2020-06-05T17:42:03Z

sdk/search/azure-search-documents/azure/search/documents/_internal/_search_client.py

        """Search the Azure search index for documents.

-        :param query: An query for searching the index
-        :type documents: str or SearchQuery
+        :param str search_text: A full-text search query expression; Use "*" or omit this parameter to


by omit this parameter - do you mean search_text is optional? If not, is it by design to not make it optional and use '*' as default value.

search method only takes search_text

0e8db7f

xiangyan99 requested a review from heaths June 3, 2020 22:57

xiangyan99 requested review from lmazuel and mayurid as code owners June 3, 2020 22:57

xiangyan99 and others added 2 commits June 3, 2020 19:36

update

2cb3750

Add AzureCliCredential and VSCodeCredential to public API (#11790) (#…

da6d4f5

…11805) Co-authored-by: Charles Lowell <chlowe@microsoft.com>

xiangyan99 requested review from chlowell and schaabs as code owners June 4, 2020 02:43

xiangyan99 and others added 2 commits June 3, 2020 19:43

Merge branch 'master' into search_search_method

902db93

xiangyan99 requested review from amishra-dev, annatisch, iscai-msft, kristapratico, rakshith91, shurd, xiafu-msft, YijunXieMS, yunhaoling, zezha-msft and zikalino as code owners June 4, 2020 18:09

xiangyan99 added 9 commits June 4, 2020 11:10

Merge branch 'master' into search_search_method

bf94e32

rename AnalyzeText to AnalyzeTextRequest

88f8895

fixes

25a0755

update

d4cf252

update changelog

6212964

update

af02a00

add types

0c3f697

update

6376587

rename batch methods

a3f66a8

xiangyan99 removed request for iscai-msft, yunhaoling, YijunXieMS and xiafu-msft June 5, 2020 01:40

update

13d56f4

heaths previously approved these changes Jun 5, 2020

View reviewed changes

update

a544404

xiangyan99 dismissed heaths’s stale review via a544404 June 5, 2020 04:51

xiangyan99 requested review from heaths and tg-msft June 5, 2020 05:04

tg-msft previously approved these changes Jun 5, 2020

View reviewed changes

rakshith91 reviewed Jun 5, 2020

View reviewed changes

updates

a6d0fa7

xiangyan99 dismissed tg-msft’s stale review via a6d0fa7 June 5, 2020 17:51

rakshith91 approved these changes Jun 5, 2020

View reviewed changes

xiangyan99 merged commit 5c4dc31 into master Jun 5, 2020

xiangyan99 deleted the search_search_method branch June 5, 2020 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

code refactoring 4 #11800

code refactoring 4 #11800

xiangyan99 commented Jun 3, 2020

heaths left a comment

heaths Jun 5, 2020

xiangyan99 Jun 5, 2020

xiangyan99 commented Jun 5, 2020

xiangyan99 commented Jun 5, 2020

azure-pipelines bot commented Jun 5, 2020

xiangyan99 commented Jun 5, 2020

azure-pipelines bot commented Jun 5, 2020

tg-msft left a comment

tg-msft Jun 5, 2020

xiangyan99 Jun 5, 2020

tg-msft Jun 5, 2020

xiangyan99 Jun 5, 2020

tg-msft Jun 5, 2020

rakshith91 Jun 5, 2020

rakshith91 Jun 5, 2020 •

edited

Loading

rakshith91 Jun 5, 2020

rakshith91 Jun 5, 2020

rakshith91 Jun 5, 2020 •

edited

Loading

		client = SearchIndexClient(endpoint="<service endpoint>"
		credential=credential)

	and [api key](https://docs.microsoft.com/en-us/azure/search/search-security-api-keys) you can create the Search Service client:
	and [api key](https://docs.microsoft.com/en-us/azure/search/search-security-api-keys) you can create the Search Index client:


		credential = AzureKeyCredential("<api key>")

		client = SearchIndexerClient(endpoint="<service endpoint>"

code refactoring 4 #11800

code refactoring 4 #11800

Conversation

xiangyan99 commented Jun 3, 2020

heaths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangyan99 commented Jun 5, 2020

xiangyan99 commented Jun 5, 2020

azure-pipelines bot commented Jun 5, 2020

xiangyan99 commented Jun 5, 2020

azure-pipelines bot commented Jun 5, 2020

tg-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakshith91 Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakshith91 Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

rakshith91 Jun 5, 2020 •

edited

Loading

rakshith91 Jun 5, 2020 •

edited

Loading