-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Better names for assertions #14070
Conversation
dataSources: EntityDataSource[]; | ||
normalizedText?: string; | ||
} | ||
|
||
// @public (undocumented) | ||
export interface HealthcareEntityAssertion { | ||
association?: HealthcareEntityAssociation; |
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.
EntityAssociation
should be good enough, since it is under the HealthcareEntityAssertion
. That logic also applies to EntityCertainty
and EntityConditionality
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 already use the Healthcare
prefix for many types, e.g.:
HealthcareEntityRelation,
HealthcareEntityRelationRole,
HealthcareEntityRelationRoleType
So I think we should be consistent and use the prefix Healthcare
.
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.
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.
I don't necessarily agree. The relations, roles, and types, are in fact specific to Healthcare.
The association, certainty, and conditionally could apply to a general entity. I understand it is not the case now, but it will be good to ask the service team if this concept could ever be applied to other entities/endpoints
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.
Ok I asked Ashly and he said there could be plans to add assertions for other type of entities in one or two semesters. Based on this, I will remove the Healthcare prefix.
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.
Thanks for the follow up Deya!
} | ||
|
||
// @public | ||
export type HealthcareEntityAssociation = "subject" | "other"; |
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.
Does JS/TS need to capitalize the first letter, such as "Subject", "Other"?
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.
I am just following the swagger, does this needs to be fixed in the swagger?
Implements new naming for assertions based on @mssfang suggestions. I am using the prefix Healthcare to conform to the naming for healthcare types. I am also fixing a small issue in the definition for
Certainty
(fixed in the swagger Azure/azure-rest-api-specs#13247).