Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added HelpDialog, TranslateDialog. Deleted IAmDialog. Renamed ChitChat to None. Added UserEmotion to UserInfoDialog #24

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kpstrawn
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@jsy1218-zz jsy1218-zz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We need to change back from NoneDialog to ChitChatDialog class name
  2. We need to evaluate all the introduced intents, how to handle them in the code
  3. We need to evaluate if IAm intent should be deleted. I feel this is one of the most important intents.

@@ -9,13 +9,14 @@

namespace DTML.EduBot.Dialogs
{
public partial class ChitChatDialog : QnaLuisDialog<object>
public partial class NoneDialog : QnaLuisDialog<object>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class name should not be changed to NoneDialog. Right now ChitChatDialog reflects the fact the bot will try its best to respond.

Plus ChitChatDialog is a partial class that is shared for all different intents.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we rename this? did we change the functionality from chit chat, to handling only "None" intents?

{
[LuisIntent("Greeting")]
public async Task Greeting(IDialogContext context, LuisResult result)
{
string botresponse = BotPersonality.GetRandomGreeting();
await context.PostAsync(botresponse);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove blank space

[LuisIntent("Help")]
public async Task HandleHelpIntent(IDialogContext context, LuisResult result)
{
await context.PostAsync("");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-empty response. This is for user experience.


namespace DTML.EduBot.Dialogs
{
public partial class ChitChatDialog : QnaLuisDialog <Object>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why IAm intent got removed?

{
await context.SayAsync(BotPersonality.BotResponseToGibberish, BotPersonality.BotResponseToGibberish);
}
//[LuisIntent("Gibberish")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why commenting out? If not in use, we should delete the code block.

With that said, should this code block still function?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a part of the code that is commented out is called "Dead Code". I assume this was left over while testing the code. Dead code should be removed before code check in.

{
public partial class HelpDialog : QnaLuisDialog<object>
{
[LuisIntent("Translate")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Translate intent? Right now we have a translate API in place. Plus, if the user needs help, it should be handled by help intent, followed by yes/no choice, and then we call translate API.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use translate intent to explicitly ask for translation of a phase. e.g. while learning talking to the bot in English, the student can ask for translation of a specific phrase without changing the system language.

Can we implement the call to translation api here?

[LuisIntent("UserEmotion")]
public async Task HandleUserEmotion(IDialogContext context, LuisResult result)
{
await context.PostAsync("");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you plan to handle UserEmotion? Reply with Emoji?

Copy link
Owner

@eklavyamirani eklavyamirani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work so far. Needs a few changes before we can merge this (check comments)

using System.Linq;
using System.Web;

namespace DTML.EduBot.Common
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be in Models, not Common

@@ -12,7 +12,30 @@ public static class BotEntities
public static string Time = "time";
public static string Date = "date";
public static string Color = "color";
public static string Subject = "subject";
public static string Subject = "Subject";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static string -> const string

@@ -9,13 +9,14 @@

namespace DTML.EduBot.Dialogs
{
public partial class ChitChatDialog : QnaLuisDialog<object>
public partial class NoneDialog : QnaLuisDialog<object>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we rename this? did we change the functionality from chit chat, to handling only "None" intents?

{
await context.SayAsync(BotPersonality.BotResponseToGibberish, BotPersonality.BotResponseToGibberish);
}
//[LuisIntent("Gibberish")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a part of the code that is commented out is called "Dead Code". I assume this was left over while testing the code. Dead code should be removed before code check in.

{
public partial class HelpDialog : QnaLuisDialog<object>
{
[LuisIntent("Translate")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use translate intent to explicitly ask for translation of a phase. e.g. while learning talking to the bot in English, the student can ask for translation of a specific phrase without changing the system language.

Can we implement the call to translation api here?

[LuisIntent("Translate")]
public async Task HandleTranslateIntent(IDialogContext context, LuisResult result)
{
await context.PostAsync("");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the await context.PostAsync(""); statement should send back non empty messages before we can merge these changes. Can you change these?

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants