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] Adding analyze and healthcare APIs #11375

Merged
merged 13 commits into from
Nov 23, 2020

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Sep 21, 2020

Implements healthcare and analyze APIs which are LROs and return paginated results.

TODO:
- Update CHANGELOG.md
- Update README.md.
- Add samples.

@deyaaeldeen deyaaeldeen force-pushed the regen-ta-3.2-preview.1 branch 4 times, most recently from 8052095 to d93c161 Compare October 8, 2020 16:34
Copy link
Contributor

@willmtemple willmtemple left a 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.

options: HealthStatusOptions = {}
): Promise<PagedAsyncIterableHealthEntities> {
const iter = this._listHealthcareEntities(jobId, input, options);
const pagedIterator = {
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice!

@deyaaeldeen deyaaeldeen changed the title [Text Analytics] Regen ta 3.2 preview.1 [Text Analytics] Adding analyze and healthcare APIs Nov 5, 2020
@deyaaeldeen deyaaeldeen marked this pull request as ready for review November 5, 2020 22:57
xirzec
xirzec previously requested changes Nov 6, 2020
Copy link
Member

@xirzec xirzec left a 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.

sdk/textanalytics/ai-text-analytics/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -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;
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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;
}

Copy link
Contributor

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.

// @public
export interface BeginAnalyzeHealthcareOptions {
// (undocumented)
health?: HealthcareJobOptions;
Copy link
Member

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

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 think you are discussing two things here:

  • analyze key is typed as AnalyzeJobOptions which is just a synonym to TextAnalyticsOperationOptions, so why not type it as TextAnalyticsOperationOptions 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/swagger/README.md Outdated Show resolved Hide resolved
sdk/textanalytics/ai-text-analytics/test/apiKey.spec.ts Outdated Show resolved Hide resolved
@@ -77,4 +77,449 @@ describe("[API Key] TextAnalyticsClient", function() {
assert.equal(results.length, 1);
assertAllSuccess(results);
});

describe("#health", () => {
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Contributor

@sadasant sadasant left a 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!

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.

7 participants