-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Conversation
@@ -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 |
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.
Also should we add that sphinx directive for properties that are only available for certain versions?
: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 |
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.
good point, i'll add that, it might not look great but at least we'll have it. thanks!
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.
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 |
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.
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)
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.
np, good point here you go bb
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.
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 |
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.
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): |
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.
What are u testing here? not sure I understand the purpose of the test.
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.
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): |
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.
do u know if the bingId property will always be populated?
worth adding a test checking that the property has content on it
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.
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
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.
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
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.
assert checks that it's not None
and not an empty string
…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)
…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) ...
…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) ...
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>
fixes #12998