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.

Sign up for GitHub

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

Jump to bottom

TA PagedFlux, PagedIterable, PagedResponse class #8391

Merged
merged 31 commits into from
Mar 6, 2020
Merged

TA PagedFlux, PagedIterable, PagedResponse class #8391

merged 31 commits into from
Mar 6, 2020

Conversation

mssfang
Copy link
Member

@mssfang mssfang commented Feb 21, 2020 •
edited
Loading

This PR includes:

  • API Tool feedbacks: https://apiview.dev/Assemblies/Review/942b652edaf44b7189f41270cf07e54f
  • Delete DocumentResultCollection class and create TextAnalyticsPagedResponse class to include modelVersion and TextAnalyticsBatchStatistics.
  • Implement a new TextAnalyticsPagedFlux class that takes TextAnalyticsPagedResponse. And TextAnalyticsPagedIterable fot the synchronous APIs.
  • Change all collection APIs to return TextAnalyticsPagedFlux for Async and TextAnalyticsPagedIterable for Sync APIs.
  • Default value for defaultCountryHint. "As an example, in .NET we use the default of the client and then give the option to the user to override the behavior and use None as default." If a user pass in "none" case-insencetive, we treat it as an empty string.
  • Rename offset and length to graphemeLength and graphemeOffset in SentenceSentiment,
    CategorizedEntity, PiiEntity, LinkedEntityMatch.
  • Rename showStatistics() to isIncludeStatistics() in the TextAnalyticsRequestOptions
  • Rename getErrorCodeValue() to getCode()
  • Add an enum class EntityCategory as .NET has now.
  • Fixes Text Analytics Readme description of Sentiment analysis is reflecting v2 and not v3. #8347 Text Analytics Readme description of Sentiment analysis is reflecting v2 and not v3.

fixes #8353
fixes #8224

@mssfang mssfang added Client This issue points to a problem in the data-plane of the library. TextAnalytics labels Feb 21, 2020
@mssfang mssfang added this to the [2020] March milestone Feb 21, 2020
@mssfang mssfang self-assigned this Feb 21, 2020
@mssfang mssfang mentioned this pull request Feb 21, 2020
Closed
16 tasks
mssfang added 14 commits February 22, 2020 22:56
@mssfang mssfang requested a review from iscai-msft March 5, 2020 16:49
@mssfang
Copy link
Member Author

mssfang commented Mar 5, 2020

@maririos This PR will include all changes for P3. I don't think there is a time for merging other PRs.

maririos
maririos reviewed Mar 5, 2020
View reviewed changes
...java/com/azure/ai/textanalytics/batch/RecognizedCategorizedEntitiesBatchStringDocuments.java Outdated
import java.util.List;

/**
* Sample demonstrates how to recognize the entities of a batch input text.
Copy link
Member

Choose a reason for hiding this comment

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

Consider changing to documents or batch of documents,
Same across this and other documents

maririos
maririos reviewed Mar 5, 2020
View reviewed changes
sdk/textanalytics/azure-ai-textanalytics/README.md Outdated
linkedEntityMatch.getOffset(), linkedEntityMatch.getLength(), linkedEntityMatch.getScore());
}
}
linkedEntityMatch.getGraphemeOffset(), linkedEntityMatch.getGraphemeLength(), linkedEntityMatch.getScore()));
Copy link
Member

Choose a reason for hiding this comment

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

Consider not showing the grapheme properties in the Readme or samples

maririos
maririos reviewed Mar 5, 2020
View reviewed changes
...ure-ai-textanalytics/src/main/java/com/azure/ai/textanalytics/models/TextSentimentLabel.java Outdated
import com.fasterxml.jackson.annotation.JsonCreator;

/**
* Defines values for {@link TextSentimentLabel}.
Copy link
Member

Choose a reason for hiding this comment

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

we should drop the 'Labelhere too, so justTextSentiment`

@maririos
Copy link
Member

maririos commented Mar 5, 2020

I can't comment on that line, but please update the version of the package in here: https://github.com/Azure/azure-sdk-for-java/pull/8391/files#diff-120a7e45ac3baf2e0719201e59beb2e4R28

@mssfang mssfang marked this pull request as ready for review March 6, 2020 04:08
@mssfang mssfang requested review from maririos and samvaity March 6, 2020 04:08
@mssfang mssfang requested a review from maririos March 6, 2020 06:26
@mssfang mssfang merged commit ea763fa into Azure:master Mar 6, 2020
@mssfang mssfang deleted the TextAnalyticsPagedFlux branch March 6, 2020 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maririos maririos maririos approved these changes

@samvaity samvaity samvaity approved these changes

@kristapratico kristapratico Awaiting requested review from kristapratico

@willmtemple willmtemple Awaiting requested review from willmtemple

@iscai-msft iscai-msft Awaiting requested review from iscai-msft

Assignees

@mssfang mssfang

Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Text Analytics
Projects
None yet
Milestone
[2020] March
3 participants
@mssfang @maririos @samvaity