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

[text analytics] add bing_id property to LinkedEntity class #13446

Merged
merged 8 commits into from Sep 1, 2020

Conversation

iscai-msft
Copy link
Contributor

fixes #12998

@@ -611,6 +611,9 @@ class LinkedEntity(DictMixin):
:ivar data_source: Data source used to extract entity linking,
such as Wiki/Bing etc.
:vartype data_source: str
:param str bing_id: Bing unique identifier of the recognized entity. Use in conjunction
Copy link
Member

Choose a reason for hiding this comment

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

Also should we add that sphinx directive for properties that are only available for certain versions?

Suggested change
:param str bing_id: Bing unique identifier of the recognized entity. Use in conjunction
:ivar str bing_id: Bing unique identifier of the recognized entity. Use in conjunction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, i'll add that, it might not look great but at least we'll have it. thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have other properties that should have this versionadded docstring too, have created an issue here trackcing that

@@ -586,3 +586,19 @@ def test_string_index_type_not_fail_v3(self, client):
# make sure that the addition of the string_index_type kwarg for v3.1-preview.1 doesn't
# cause v3.0 calls to fail
client.recognize_linked_entities(["please don't fail"])

# currently only have this as playback since the dev endpoint is unreliable
@pytest.mark.playback_test_only
Copy link
Member

Choose a reason for hiding this comment

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

can we open an issue to remove playback marker when endpoint works reliably? (sorry for nit, I feel like these kinds of things are too easy to forget about)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np, good point here you go bb

kristapratico
kristapratico previously approved these changes Aug 31, 2020
Copy link
Member

@kristapratico kristapratico left a comment

Choose a reason for hiding this comment

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

LGTM

"text_analytics_account": "https://cognitiveusw2dev.azure-api.net/"
})
def test_bing_id(self, client):
# make sure that the addition of the string_index_type kwarg for v3.1-preview.1 doesn't
Copy link
Member

Choose a reason for hiding this comment

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

comment needs to be updated

"text_analytics_account_key": os.environ.get('AZURE_TEXT_ANALYTICS_KEY'),
"text_analytics_account": "https://cognitiveusw2dev.azure-api.net/"
})
def test_bing_id(self, client):
Copy link
Member

Choose a reason for hiding this comment

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

What are u testing here? not sure I understand the purpose of the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one is just to check that I've correctly set the bing ID I get back from the service. I didn't tie it to the actual bing ID returned from the service though, I'm not sure how arbitrary that number is

"text_analytics_account_key": os.environ.get('AZURE_TEXT_ANALYTICS_KEY'),
"text_analytics_account": "https://cognitiveusw2dev.azure-api.net/"
})
def test_bing_id(self, client):
Copy link
Member

Choose a reason for hiding this comment

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

do u know if the bingId property will always be populated?
worth adding a test checking that the property has content on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be populated by the server for v3.1-preview.2 and up (it's not shown as an optional parameter). I'm confused by "worth adding a test checking that the property has content on it", I believe that's what I'm checking in this test

Copy link
Member

Choose a reason for hiding this comment

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

It might be me and not knowing ton of Python, but assert entity doesn't seem to be checking that bing_id has something on it. like it is not None or null, or empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert checks that it's not None and not an empty string

kristapratico
kristapratico previously approved these changes Aug 31, 2020
@iscai-msft iscai-msft merged commit 9b1b9ec into Azure:master Sep 1, 2020
@iscai-msft iscai-msft deleted the add_bing_id branch September 1, 2020 14:53
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Sep 1, 2020
…into add_redacted_text

* 'master' of https://github.com/Azure/azure-sdk-for-python:
  [text analytics] add bing_id property to LinkedEntity class (Azure#13446)
  fix typing for paging methods (Azure#13410)
  [text analytics] add domain_filter param (Azure#13451)
  fix issue Azure#11658 for is_valid_resource_id (Azure#11709)
  added create_table_if_not_exists method to table service client (Azure#13385)
  [ServiceBus] Test and failure improvements (Azure#13345)
  Proper encoding and decoding of source URLs - Fixes special characters in source URL issue (Azure#13275)
  Switch retry (Azure#13264)
  [ServiceBus] ServiceBusClient close spawned children (Azure#13077)
  fixing version issue by not overwriting the version with the semantic… (Azure#13411)
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Sep 1, 2020
…into expose_parse_vault_id

* 'master' of https://github.com/Azure/azure-sdk-for-python: (37 commits)
  [text analytics] add versionadded sphinx documentation (Azure#13450)
  [text analytics] add bing_id property to LinkedEntity class (Azure#13446)
  fix typing for paging methods (Azure#13410)
  [text analytics] add domain_filter param (Azure#13451)
  fix issue Azure#11658 for is_valid_resource_id (Azure#11709)
  added create_table_if_not_exists method to table service client (Azure#13385)
  [ServiceBus] Test and failure improvements (Azure#13345)
  Proper encoding and decoding of source URLs - Fixes special characters in source URL issue (Azure#13275)
  Switch retry (Azure#13264)
  [ServiceBus] ServiceBusClient close spawned children (Azure#13077)
  fixing version issue by not overwriting the version with the semantic… (Azure#13411)
  clean up reference and tests (Azure#13412)
  Consistent returns (Azure#13245)
  [text analytics] return None for offset and length for v3.0 (Azure#13382)
  edit all authentication files and add a test (Azure#13355)
  Add managed_identity_client_id argument to DefaultAzureCredential (Azure#13218)
  [text analytics] add string-index-type support (Azure#13378)
  [text analytics] fix error response if pii entities is called from v3.0 client (Azure#13383)
  Send spec (Azure#13143)
  Anomaly Detector 3.0.0b2 release (Azure#13351)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Sep 2, 2020
…into link_om_sample

* 'master' of https://github.com/Azure/azure-sdk-for-python: (23 commits)
  Int32 serialization (Azure#13452)
  add output to opinion mining sample (Azure#13494)
  Add Document w/ Eng Sys Checks (Azure#13492)
  update version (Azure#13495)
  Remove resources post test (Azure#13379)
  bing_id -> bing_entity_search_api_id (Azure#13491)
  [EventGrid] Read me + improve docstrings (Azure#13484)
  Build AuthenticationRecords from ADFS identity tokens (Azure#13341)
  Support Subject Name/Issuer authentication (Azure#13350)
  Add KeyVaultAccessControlClient for data plane RBAC (Azure#13372)
  [text analytics] Add redacted_text (Azure#13449)
  add python sdk sample (Azure#13338)
  [text analytics] add versionadded sphinx documentation (Azure#13450)
  [text analytics] add bing_id property to LinkedEntity class (Azure#13446)
  fix typing for paging methods (Azure#13410)
  [text analytics] add domain_filter param (Azure#13451)
  fix issue Azure#11658 for is_valid_resource_id (Azure#11709)
  added create_table_if_not_exists method to table service client (Azure#13385)
  [ServiceBus] Test and failure improvements (Azure#13345)
  Proper encoding and decoding of source URLs - Fixes special characters in source URL issue (Azure#13275)
  ...
rakshith91 pushed a commit to rakshith91/azure-sdk-for-python that referenced this pull request Sep 4, 2020
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Mar 19, 2021
Add the march public preview API version of extended location (Azure#13446)

* Add the march public preview API version of extended location

* Tweak: remove references to iot

* Address comments from Phoenix by removing 500 references and adding back the default version tag

* Revert privatepreview changes

* Tweak: remove "500" from the swagger itself

* Tweak: remove dash from readme

* fix python pipeline

* Tweak: run prettier against the repo folder

* Tweak: remove 404 from customlocations.json and CustomLocationsGet.json

* Tweak: csharp README sdk output folder path to fix downstream generation issue

Co-authored-by: 00Kai0 <sunkaihuisos@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[text analytics] add bing ID property for LinkedEntity
4 participants