Skip to content
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

update version number and API_version support #12154

Merged
merged 7 commits into from
Jun 25, 2020
Merged

Conversation

xiangyan99
Copy link
Member

No description provided.

@xiangyan99
Copy link
Member Author

/azp run python - search - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@xiangyan99
Copy link
Member Author

#12171

@xiangyan99 xiangyan99 changed the title update update version number and API_version support Jun 25, 2020
heaths
heaths previously approved these changes Jun 25, 2020
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to keep you unblocked from making fixes and submitting, but please see comments. I also see now this was to change the package version and not the API version, but please note that is also necessary.

# ------------------------------------

_SUPPORTED_API_VERSIONS = [
"2019-05-06-Preview",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the PR correctly, it's to make sure this is "2020-06-03" everywhere.

Copy link
Member Author

@xiangyan99 xiangyan99 Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have a separate PR to update the API version number (that will include updating auto-generated code)

@@ -138,6 +142,68 @@ def search(self, search_text, **kwargs):

:param str search_text: A full-text search query expression; Use "*" or omit this parameter to
match all documents.
:keyword include_total_result_count: A value that specifies whether to fetch the total count of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tg-msft, shouldn't this be include_total_count? Or was that one of the things we were asked to change? I don't see it changed in .NET yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include_total_result_count is from the swagger. Do we want to change it?

@@ -266,6 +367,36 @@ def autocomplete(self, search_text, suggester_name, **kwargs):
:param str search_text: The search text on which to base autocomplete results.
:param str suggester_name: The name of the suggester as specified in the suggesters
collection that's part of the index definition.
:keyword autocomplete_mode: Specifies the mode for Autocomplete. The default is 'oneTerm'. Use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In .NET we just have this as mode. @tg-msft, should it match in this case or is this idiomatically acceptable?

/cc @johanste

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok to use mode, just want to confirm because autocomplete_mode is from swagger.

@@ -75,6 +78,7 @@ class SearchClient(HeadersMixin):
def __init__(self, endpoint, index_name, credential, **kwargs):
# type: (str, str, AzureKeyCredential, **Any) -> None

get_api_version(kwargs, "2019-05-06-Preview")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess here it's used has a "check", then I would split a "check" and a "get" into two different methods, with "get" calling check first. I find it easier to read intent

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's optional feedback, I let you decide if that makes the code less efficient

@@ -45,6 +47,9 @@ class SearchClient(HeadersMixin):
def __init__(self, endpoint, index_name, credential, **kwargs):
# type: (str, str, AzureKeyCredential, **Any) -> None

api_version = kwargs.pop('api_version', None)
if api_version:
check_api_version(api_version)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a lot the pattern "if api_version: check", does it mean check should not raise if api_version is None? And then simplify all those occurences?

lmazuel
lmazuel previously approved these changes Jun 25, 2020
Copy link
Member

@lmazuel lmazuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the Python side of things, no feedback on Search semantic



def get_api_version(kwargs, default):
# type: (Dict[str, Any]) -> str
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the type hint correct? there are 2 params above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Fixed.

@xiangyan99 xiangyan99 merged commit ace79c0 into master Jun 25, 2020
@xiangyan99 xiangyan99 deleted the search_review_feedback branch June 25, 2020 21:56
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jun 30, 2020
…into regenerate_certs

* 'master' of https://github.com/Azure/azure-sdk-for-python: (27 commits)
  Set http_logging_policy in Configuration (Azure#12218)
  Sync eng/common directory with azure-sdk-tools repository (Azure#11990)
  AzureCliCredential instructs CLI not to color output (Azure#11362)
  Sdk automation/track2 azure mgmt storage (Azure#12238)
  Fix changelog of CustomVision (Azure#12225)
  Doc update for conf file name (Azure#12224)
  update doc for content_type (Azure#12220)
  update API version to use 2020-06-30 (Azure#12208)
  Use MSAL's custom transport API (Azure#11892)
  add breadcumbs for training filter (Azure#12196)
  [formrecognizer] arch board feedback renames (Azure#12207)
  Dataplane autogeneration (Azure#12210)
  update version number and API_version support (Azure#12154)
  Update SECURITY.md (Azure#12209)
  adding a pip freeze to ensure we fully understand what our environment has (Azure#12173)
  FaceAPI 0.4.1 (Azure#12199)
  [ServiceBus] Enable Async Unittest (Azure#12001)
  add search owver (Azure#12190)
  update training docstring (Azure#12168)
  don't use mgmt track2 (Azure#12183)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jun 30, 2020
…into regenerate_keys

* 'master' of https://github.com/Azure/azure-sdk-for-python:
  Set http_logging_policy in Configuration (Azure#12218)
  Sync eng/common directory with azure-sdk-tools repository (Azure#11990)
  AzureCliCredential instructs CLI not to color output (Azure#11362)
  Sdk automation/track2 azure mgmt storage (Azure#12238)
  Fix changelog of CustomVision (Azure#12225)
  Doc update for conf file name (Azure#12224)
  update doc for content_type (Azure#12220)
  update API version to use 2020-06-30 (Azure#12208)
  Use MSAL's custom transport API (Azure#11892)
  add breadcumbs for training filter (Azure#12196)
  [formrecognizer] arch board feedback renames (Azure#12207)
  Dataplane autogeneration (Azure#12210)
  update version number and API_version support (Azure#12154)
  Update SECURITY.md (Azure#12209)
  adding a pip freeze to ensure we fully understand what our environment has (Azure#12173)
  FaceAPI 0.4.1 (Azure#12199)
  [ServiceBus] Enable Async Unittest (Azure#12001)
  add search owver (Azure#12190)
  update training docstring (Azure#12168)
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jun 30, 2020
…into regenerate_secrets

* 'master' of https://github.com/Azure/azure-sdk-for-python: (36 commits)
  Install dev dependency when running apistub (Azure#12268)
  SharedTokenCacheCredential lazily loads the cache (Azure#12172)
  Changes in docs [Form Recognizer] (Azure#12216)
  [formrecognizer] adjust text angle to fit in specified interval (Azure#12248)
  Set http_logging_policy in Configuration (Azure#12218)
  Sync eng/common directory with azure-sdk-tools repository (Azure#11990)
  AzureCliCredential instructs CLI not to color output (Azure#11362)
  Sdk automation/track2 azure mgmt storage (Azure#12238)
  Fix changelog of CustomVision (Azure#12225)
  Doc update for conf file name (Azure#12224)
  update doc for content_type (Azure#12220)
  update API version to use 2020-06-30 (Azure#12208)
  Use MSAL's custom transport API (Azure#11892)
  add breadcumbs for training filter (Azure#12196)
  [formrecognizer] arch board feedback renames (Azure#12207)
  Dataplane autogeneration (Azure#12210)
  update version number and API_version support (Azure#12154)
  Update SECURITY.md (Azure#12209)
  adding a pip freeze to ensure we fully understand what our environment has (Azure#12173)
  FaceAPI 0.4.1 (Azure#12199)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants