-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[TA] Make fromString method package-private #11227
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
Changes from 3 commits
d82c62f
c22acec
9ef5406
e521c53
04087e0
5848b1d
7fc12f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,8 +15,6 @@ | |
| import com.azure.ai.textanalytics.models.TextAnalyticsRequestOptions; | ||
| import com.azure.ai.textanalytics.models.TextAnalyticsWarning; | ||
| import com.azure.ai.textanalytics.models.TextDocumentInput; | ||
| import com.azure.ai.textanalytics.models.TextSentiment; | ||
| import com.azure.ai.textanalytics.models.WarningCode; | ||
| import com.azure.ai.textanalytics.util.TextAnalyticsPagedFlux; | ||
| import com.azure.ai.textanalytics.util.TextAnalyticsPagedResponse; | ||
| import com.azure.core.exception.HttpResponseException; | ||
|
|
@@ -28,7 +26,6 @@ | |
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import static com.azure.ai.textanalytics.TextAnalyticsAsyncClient.COGNITIVE_TRACING_NAMESPACE_VALUE; | ||
|
|
@@ -144,43 +141,21 @@ private TextAnalyticsPagedResponse<AnalyzeSentimentResult> toTextAnalyticsPagedR | |
| * @return The {@link AnalyzeSentimentResult} to be returned by the SDK. | ||
| */ | ||
| private AnalyzeSentimentResult convertToAnalyzeSentimentResult(DocumentSentiment documentSentiment) { | ||
| // Document text sentiment | ||
| final TextSentiment documentSentimentLabel = TextSentiment.fromString( | ||
| documentSentiment.getSentiment().toString()); | ||
| if (documentSentimentLabel == null) { | ||
| // Not throw exception for an invalid Sentiment type because we should not skip processing the | ||
| // other response. It is a service issue. | ||
| logger.logExceptionAsWarning( | ||
| new RuntimeException(String.format(Locale.ROOT, "'%s' is not valid text sentiment.", | ||
| documentSentiment.getSentiment()))); | ||
| } | ||
|
|
||
| final SentimentConfidenceScorePerLabel confidenceScorePerLabel = documentSentiment.getConfidenceScores(); | ||
|
|
||
| // Sentence text sentiment | ||
| final List<SentenceSentiment> sentenceSentiments = documentSentiment.getSentences().stream() | ||
| .map(sentenceSentiment -> { | ||
| final TextSentiment sentenceSentimentLabel = TextSentiment.fromString( | ||
| sentenceSentiment.getSentiment().toString()); | ||
| if (sentenceSentimentLabel == null) { | ||
| // Not throw exception for an invalid Sentiment type because we should not skip processing the | ||
| // other response. It is a service issue. | ||
| logger.logExceptionAsWarning( | ||
| new RuntimeException(String.format(Locale.ROOT, "'%s' is not valid text sentiment.", | ||
| sentenceSentiment.getSentiment()))); | ||
| } | ||
| final SentimentConfidenceScorePerLabel confidenceScorePerSentence = | ||
| sentenceSentiment.getConfidenceScores(); | ||
|
|
||
| return new SentenceSentiment(sentenceSentiment.getText(), | ||
| sentenceSentimentLabel, | ||
| sentenceSentiment.getSentiment().toString(), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to check for nulls anymore?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| new SentimentConfidenceScores(confidenceScorePerSentence.getNegative(), | ||
| confidenceScorePerSentence.getNeutral(), confidenceScorePerSentence.getPositive())); | ||
| }).collect(Collectors.toList()); | ||
|
|
||
| // Warnings | ||
| final List<TextAnalyticsWarning> warnings = documentSentiment.getWarnings().stream().map( | ||
| warning -> new TextAnalyticsWarning(WarningCode.fromString(warning.getCode().toString()), | ||
| warning -> new TextAnalyticsWarning(warning.getCode().toString(), | ||
| warning.getMessage())).collect(Collectors.toList()); | ||
|
|
||
| return new AnalyzeSentimentResult( | ||
|
|
@@ -189,7 +164,7 @@ private AnalyzeSentimentResult convertToAnalyzeSentimentResult(DocumentSentiment | |
| ? null : toTextDocumentStatistics(documentSentiment.getStatistics()), | ||
| null, | ||
| new com.azure.ai.textanalytics.models.DocumentSentiment( | ||
| documentSentimentLabel, | ||
| documentSentiment.getSentiment().toString(), | ||
| new SentimentConfidenceScores( | ||
| confidenceScorePerLabel.getNegative(), | ||
| confidenceScorePerLabel.getNeutral(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ public final class WarningCode extends ExpandableStringEnum<WarningCode> { | |
| * @return The corresponding {@link WarningCode}. | ||
| */ | ||
| @JsonCreator | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't need this>
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WarningCode is not a generated model. if you take a look at what Speaking of what other SDK's behavior of including this annotation. They included it as well.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be included on the deserialization layer i.e on the service side equivalent model. Don't need it here. |
||
| public static WarningCode fromString(String name) { | ||
| static WarningCode fromString(String name) { | ||
| return fromString(name, WarningCode.class); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Redundant null check that catched in the code quality process.
[ERROR] Redundant nullcheck of documentSentimentLabel, which is known to be non-null in com.azure.ai.textanalytics.AnalyzeSentimentAsyncClient.convertToAnalyzeSentimentResult(DocumentSentiment) [com.azure.ai.textanalytics.AnalyzeSentimentAsyncClient] Redundant null check at AnalyzeSentimentAsyncClient.java:[line 147] RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE
[ERROR] Redundant nullcheck of sentenceSentimentLabel, which is known to be non-null in com.azure.ai.textanalytics.AnalyzeSentimentAsyncClient.lambda$convertToAnalyzeSentimentResult$9(SentenceSentiment) [com.azure.ai.textanalytics.AnalyzeSentimentAsyncClient] Redundant null check at AnalyzeSentimentAsyncClient.java:[line 161] RCN_REDUNDANT_NULLCHECK_OF_NONNULL_
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 started coming up from the current change?
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.
Yes. Null checking here is an redundant check. As the error message explained here.