-
Notifications
You must be signed in to change notification settings - Fork 288
M3/M4 samples #208
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
M3/M4 samples #208
Conversation
Pull Request Test Coverage Report for Build 391
💛 - Coveralls |
@CezaryMarcjan and @DeniseMak, can you take a look at the dispatch sample and let me know if you have any feedback regarding it? Per the original email thread that outlines the samples to be added to the repo, I was planning on going back in and re-adding the QnAMaker KBID and Subscription Key for quick spin-up as QnAMaker does not have a "public" setting at this time. |
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 but I'm not JS expert...
if (context.activity.type === 'conversationUpdate' && context.activity.membersAdded[0].name !== 'Bot') { | ||
return context.sendActivity(`Hello "${context.activity.membersAdded[0].name}"!`); | ||
} | ||
if (context.activity.type === 'message') { |
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.
if [](start = 8, length = 2)
why not an "else if" ?
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 I learned that from someone at CSE... But I will change it 👍
adapter.processActivity(req, res, (context) => { | ||
// On "conversationUpdate"-type activities this bot will send a greeting message to users joining the conversation. | ||
if (context.activity.type === 'conversationUpdate' && context.activity.membersAdded[0].name !== 'Bot') { | ||
return context.sendActivity(`Hello "${context.activity.membersAdded[0].name}"!`); |
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.
return [](start = 12, length = 6)
I think we've decided to use async/await in all the samples
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 recall that now, I'll go in and change this.
samples/dispatch-es6/app.js
Outdated
|
||
// create conversation state | ||
const conversationState = new ConversationState(new MemoryStorage()); | ||
adapter.use(conversationState); |
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.
Cash and I have decided that we should always show the use of both UserState() and ConversationState() in all samples with state. I can send you the general boilerplate to include
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've added the boilerplate to all the samples that were already using ConversationState
, the only sample I didn't add it to was the conversationUpdate sample.
break; | ||
default: | ||
await dc.begin('None'); | ||
} |
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 should be restructured a bit. I'll talk to you about this
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 believe I covered the changes we discussed, which was centered on how to call dc.continue()
; I based the new changes on the sample code in samples/dialogs/alarmbot-ts
.
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.
For the dispatch sample, we need to decide what to do about the intent names being changed to have an underscore in the recognizer result. Should we just convert the "_" to a "."?
Also, it would be good to show in one of the samples how to use dialogs with LUIS recognizer middleware, since the middleware runs on every turn but dialogs can bypass the intent-checking logic.
Add new M3 samples:
Cleanup: