-
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] Adding analyze and healthcare APIs #11375
[Text Analytics] Adding analyze and healthcare APIs #11375
Conversation
b372815
to
d1c4b37
Compare
sdk/textanalytics/ai-text-analytics/src/generated/lro/constants.ts
Outdated
Show resolved
Hide resolved
8052095
to
d93c161
Compare
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.
Health poller implementation looks good to me, with a couple of notes about some things that have changed recently, or some minor nitpicks/readability optimizations you might choose to make.
sdk/textanalytics/ai-text-analytics/src/lro/health/healthPoller.ts
Outdated
Show resolved
Hide resolved
options: HealthStatusOptions = {} | ||
): Promise<PagedAsyncIterableHealthEntities> { | ||
const iter = this._listHealthcareEntities(jobId, input, options); | ||
const pagedIterator = { |
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 looks great, but I can't help but wonder if this could be replaced by something simpler like
const pagedIterator = Object.assign(this._listHealthcareEntities(jobId, input, options), {
byPage: (settings?: PageSettings) => {
const pageOptions = { ...options, top: settings?.maxPageSize };
return this._listHealthcareEntitiesPaginated(jobId, input, pageOptions);
});
since essentially all we're doing here is providing an escape to the underlying paged API and the unpaged iterator is just a convenience on top of that.
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.
nice!
e2d0d84
to
cdec7a2
Compare
sdk/textanalytics/ai-text-analytics/review/ai-text-analytics.api.md
Outdated
Show resolved
Hide resolved
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.
Looks like a really cool addition for getting bulk analysis done! I don't think I've seen a poller return a paginated result before either, so that's pretty neat.
I left a lot of developer experience feedback, though my biggest piece of feedback is: where are the samples and examples? I would expect not only some samples to get added, but also the README to demonstrate how to use this very complex API.
@@ -351,7 +533,7 @@ export interface TextDocumentStatistics { | |||
export type TokenSentimentValue = "positive" | "mixed" | "negative"; | |||
|
|||
// @public | |||
export type WarningCode = "LongWordsInDocument" | "DocumentTruncated"; | |||
export type WarningCode = "LongWordsInDocument" | "DocumentTruncated" | string; |
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.
is this for open-ended extensibility? Should we instead make this string
and have KnownWarningCodes
or something?
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.
Hey @joheredi, is this related to a recent change in the code generator? This is the swagger definition: https://github.com/Azure/azure-rest-api-specs/blob/28d37f8847253787a56d39814e7ccd5194aeeabe/specification/cognitiveservices/data-plane/TextAnalytics/preview/v3.1-preview.3/TextAnalytics.json#L815
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.
Yeah this is the way the generator is doing "extensible" enums. I don't think we have reach a formal agreement on how to generate these.
@xirzec your proposal is to instead of generating this:
export type WarningCode = "LongWordsInDocument" | "DocumentTruncated" | string;
export interface TextAnalyticsWarning {
code: WarningCode;
message: string;
}
Do this?
export type KnownWarningCode = "LongWordsInDocument" | "DocumentTruncated";
export interface TextAnalyticsWarning {
code: string;
message: string;
}
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.
The issue with this way of modeling extensible enums is that it is exactly the same as type WarningCode = string;
. The information about known values is only contained in the source code text and is fundamentally erased from the type when it comes down to set theory. Even if we added a KnownWarningCodes
enum or type, the problem then is how to associate that type with the parameter in a discoverable way. It's not going to show up when you invoke completion in an IDE, for example.
sdk/textanalytics/ai-text-analytics/review/ai-text-analytics.api.md
Outdated
Show resolved
Hide resolved
// @public | ||
export interface BeginAnalyzeHealthcareOptions { | ||
// (undocumented) | ||
health?: HealthcareJobOptions; |
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 really understand BeginAnalyzeHealthcareOptions
and BeginAnalyzeOptions
having custom subkeys that are just copies of TextAnalyticsOperationOptions
with no customization.
I would expect this to instead look like
export interface BeginAnalyzeHealthcareOptions extends TextAnalyticsOperationOptions, PollingOptions { }
or possibly
export type BeginAnalyzeHealthcareOptions = TextAnalyticsOperationOptions & PollingOptions
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 think you are discussing two things here:
analyze
key is typed asAnalyzeJobOptions
which is just a synonym toTextAnalyticsOperationOptions
, so why not type it asTextAnalyticsOperationOptions
instead? I think having a distinct relevant name makes a better user experience with intellisense and it makes it easier to extend this type in the future.- Why not flatten
BeginAnalyzeOptions
? I think keeping the options type as is makes it clear to the user which options are used to configure the analyze job itself and which are used to configure the poller.
sdk/textanalytics/ai-text-analytics/review/ai-text-analytics.api.md
Outdated
Show resolved
Hide resolved
@@ -77,4 +77,449 @@ describe("[API Key] TextAnalyticsClient", function() { | |||
assert.equal(results.length, 1); | |||
assertAllSuccess(results); | |||
}); | |||
|
|||
describe("#health", () => { |
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.
feels kinda bad this file is named apiKey.spec.ts
but it's not really about testing API keys, it's about testing client methods.
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.
Yeah, I was actually thinking of having a single test suite parameterized over the method of authentication and I created this as a first step: #11046
sdk/textanalytics/ai-text-analytics/src/lro/analyze/operation.ts
Outdated
Show resolved
Hide resolved
sdk/textanalytics/ai-text-analytics/src/lro/analyze/operation.ts
Outdated
Show resolved
Hide resolved
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.
Clean and fascinating use of core-lro with an asynchronously paginated response! Thank you for your hard work. Approved from this perspective!
2006e3f
to
e67ae4d
Compare
3a866cf
to
762ff46
Compare
Implements
healthcare
andanalyze
APIs which are LROs and return paginated results.TODO:- Update CHANGELOG.md- Update README.md.- Add samples.