Skip to content
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

Handle textless messages differently, bringing Node into parity with .NET #856

Merged
merged 4 commits into from
Apr 26, 2019

Conversation

v-kydela
Copy link
Contributor

@v-kydela v-kydela commented Apr 4, 2019

Fixes microsoft/botbuilder-dotnet#1349

Description

While there wasn't necessarily a bug in this repo, there was potential for LuisRecognizer to make a needless call to a LUIS endpoint when an utterance was empty. This PR gets the behavior of LuisRecognizer to match a recent V4 .NET PR which in turn is based off of V3 .NET. Now the recognize function will check if an utterance is empty and then create a fake result instead of getting a result from the endpoint in that case.

Specific Changes

  • LuisRecognizer.recognize now creates a RecognizerResult with a blank intent if the activity's text is null or whitespace
  • The EmptyText.json test in luisRecognizer.test.js now checks the actual RecognizerResult instead of just the topIntent function because topIntent converts the empty intent to None

Testing

The EmptyText test has been modified to fit the new behavior.

@coveralls
Copy link

coveralls commented Apr 4, 2019

Pull Request Test Coverage Report for Build #2230

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 85.729%

Totals Coverage Status
Change from base Build #2229: 0.02%
Covered Lines: 3260
Relevant Lines: 3665

💛 - Coveralls

Copy link
Contributor

@cleemullins cleemullins left a comment

Choose a reason for hiding this comment

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

This is great, but could you:

  1. Do the linting changes, and only the linting changes, in a PR.
  2. Once that's merged in, make the bug fix.

Looking through this PR I can't tell what's new and what's just a linting change, as the changeset is complex.

@v-kydela v-kydela mentioned this pull request Apr 9, 2019
@v-kydela
Copy link
Contributor Author

v-kydela commented Apr 9, 2019

@cleemullins - All right, I went ahead and did the linting in its own PR and requested you as a reviewer: #867

I must have idly auto-formatted the document without realizing how much actually got changed

@cleemullins
Copy link
Contributor

@v-kydela When I look at the Diff of this PR, I still see all the linting changes. I did merge in the separate PR with those changes earlier this evening, so I'm surprised to see them still in this PR.

Can you take a look?

@v-kydela
Copy link
Contributor Author

@cleemullins - It looks to me like your merge successfully removed the linting changes from this PR:
image

Can you look again? Maybe clear your cache or something? If you need me to change this PR then I could try a rebase but that could get messy. Then of course the last resort would be to discard this PR and just create a new one.

@cleemullins cleemullins merged commit 566f964 into master Apr 26, 2019
@cleemullins cleemullins deleted the v-kydela/textlessMessages branch April 26, 2019 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LuisRecognizer can't handle null or empty text
4 participants