-
Notifications
You must be signed in to change notification settings - Fork 528
Discovery patch #834
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
Discovery patch #834
Conversation
mkistler
left a comment
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 we want builders for all the new models.
| features.setEntities(entities); | ||
| features.setKeywords(keywords); | ||
| features.setSemanticRoles(semanticRoles); | ||
| EnrichmentOptions options = new EnrichmentOptions(); |
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 we probably want builders generated for EnrichmentOptions, NluEnrichmentFeatures, NluEnrichmentSemanticRoles, NluEnrichmentKeywords, NluEnrichmentEntities, NluEnrichmentEmotion, and NluEnrichmentSentiment.
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.
Agreed. I'll look at making that change in the swagger and updating this.
Codecov Report
@@ Coverage Diff @@
## develop #834 +/- ##
=============================================
- Coverage 56.42% 56.17% -0.25%
Complexity 2201 2201
=============================================
Files 488 496 +8
Lines 11832 12014 +182
Branches 672 674 +2
=============================================
+ Hits 6676 6749 +73
- Misses 4770 4879 +109
Partials 386 386
Continue to review full report at Codecov.
|
|
I think this is going to break existing users. 🔥 |
|
Unfortunately I think so too 😞 |
mkistler
left a comment
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 good. Two small changes needed, but I have approved to avoid the delay of another review.
| * @param targets the new targets | ||
| * @return the NluEnrichmentEmotion builder | ||
| */ | ||
| public Builder addTargets(String targets) { |
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 need "x-item-name": "target" on this array so that we get
public Builder addTarget(String target) {
We could probably make this change by hand here, rather than regenerate the whole SDK -- as long as we make sure to fix the swagger for future iterations.
| * @param targets the new targets | ||
| * @return the NluEnrichmentSentiment builder | ||
| */ | ||
| public Builder addTargets(String targets) { |
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 need "x-item-name": "target" on this array so that we get
public Builder addTarget(String target) {
We could probably make this change by hand here, rather than regenerate the whole SDK -- as long as we make sure to fix the swagger for future iterations.
Fixes #826
The Discovery service has moved to using NLU-based enrichment options. This PR makes those corresponding model changes.