-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add generativeDirectAnswer service #263
Conversation
J=CLIP-1226 TEST=auto,manual ran and saw jest test pass. tested the gda endpoint locally via core test site, saw appropriate api resp onse.
Pull Request Test Coverage Report for Build 9369880764Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9369891182Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9370359911Details
💛 - Coveralls |
should have been covered in api response validator
Pull Request Test Coverage Report for Build 9370662744Details
💛 - Coveralls |
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.
LGTM! But will defer to Clippy. Only have a nit
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.
It all seems fine to me but I would definitely like for Yen to TAL
Pull Request Test Coverage Report for Build 9372673931Details
💛 - Coveralls |
/** The text of the user-written query that prompted Search results. */ | ||
searchTerm: string, | ||
/** The complete set of Search Results */ | ||
results: VerticalResults | Record<string, VerticalResults[]> |
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.
perhaps I missed something, but why is the type a map of key to results? why not just an array of VerticalResults?
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 defined results in GDAApiRequestBody to be of type struct, so we can't pass in array. VerticalResults
is for a Vertical Search Result, Record<string, VerticalResults[]>
is for a Universal Search Result.
I think there is another way I could accomplish this: keep results to be of type VerticalResults[]
, then in GDAServiceImpl
, convert request.results to object before sending the api request. Something like this:
let results: Object = {}
if (request.results.length === 1) {
results = request.results[1]
} else if (request.results.length > 1) {
results = {verticals: request.results}
}
let me know if you prefer this way, then I can change and test it
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 think that would work better so user of core don't have to transform the universal results themselves
Pull Request Test Coverage Report for Build 9389260743Details
💛 - Coveralls |
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.
are you going to publish a new version or wait for search's publish cycle? If it's the former, then you should bump the version in package.json
Pull Request Test Coverage Report for Build 9389682713Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9391319665Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9391326582Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9403239505Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9403250729Details
💛 - Coveralls |
Add generativeDirectAnswer service (#263) J=CLIP-1226 TEST=auto,manual Co-authored-by: Yen Truong <[email protected]> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
This reverts commit e2fe1fd.
Add generativeDirectAnswer service (#263) J=CLIP-1226 TEST=auto,manual Co-authored-by: Yen Truong <[email protected]> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
J=CLIP-1226
TEST=auto,manual
ran and saw jest test pass.
tested the gda endpoint locally via core test site, saw appropriate api resp
onse.