-
Notifications
You must be signed in to change notification settings - Fork 479
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
Fix: Can pass QnAMakerOptions into both constructor and QnAMaker.GetAnswersAsync() w/o unintentional overwrite #1288
Conversation
Pull Request Test Coverage Report for Build 48056
💛 - 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.
This looks great to me, but I would like a 2nd set of eyes (John? Steven?) before merging it in. I'm still a bit out-of-it from recent medical events, so a 2nd detailed review is good.
@stevengum I think you meant put the if-statement conditions into In any case, obliterated and removed helpers And if-statement conditions are in |
@stevengum post meeting ping! :) Made changes on the edits you suggested. Please review |
Just waiting on @Zerryth to fix the breaking changes and not return a |
Awesome, thank you John & Steven. Just ran the code I had locally without making any changes to the commit and 7 of 22 tests are failing now (why me???) Once I get passed this shenanigans, I'll work on the param name/error throwing :) |
…l -- more helpful to user
Feedback addressed. NEeding to merge.
Fixes #1239, #1286
Description
QnAMakerOptions
intoQnAMaker
constructor andQnAMaker.GetAnswersAsync()
without accidentally overwriting the options passed into the constructorValidateOptions()
(DRY)Specific Changes
Can pass
QnAMakerOptions
intoQnAMaker
constructor andQnAMaker.GetAnswersAsync()
without accidentally overwriting the options passed into the constructorHydrateOptions()
to combine options passed into the constructor andGetAnswersAsync()
callScoreThreshold = 0.5F
and inGetAnswersAsync()
I provide an addition option that I wantTop = 3
results, your options in the request to theQnAMaker
service will specify bothScoreThreshold = 0.5F
andTop = 3
ScoreThreshold = 0.5F
to defaultScoreThreshold = 0.3F
and take only theGetAnswersAsync()
optionExercised clean coding practice of abstracting lower-level detailed processes into helper functions to make code more expressive/readable. See below for functions
GetAnswersAsync()
:HydrateOptions()
,QueryQnaServiceAsync()
,EmitTraceInfoAsync()
QueryQnaServiceAsync()
:BuildRequest()
,FormatQnaResultAsync()
BuildRequest()
:SetHeaders()