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

Spellcheck middleware added to JS repo #29

Merged
merged 4 commits into from Dec 10, 2018

Conversation

stephanbisser
Copy link
Member

I added the spellcheck middleware based on Bing Spell Check similar to the text analytics middleware @szul added...

Copy link
Contributor

@szul szul left a comment

Choose a reason for hiding this comment

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

Go ahead and update the root README to add your package information, and update the CODEOWNERS file to add your package and your GitHub username. Then I think it's fine to merge.

@garypretty What's the general consensus on logging to the console or standard out? Should this be excluded from production packages to avoid additional noise/file consumption?

@szul
Copy link
Contributor

szul commented Dec 10, 2018

:shipit:

@stephanbisser stephanbisser merged commit 6594e1d into BotBuilderCommunity:master Dec 10, 2018
@stephanbisser
Copy link
Member Author

PR merged - done

"dependencies": {
"azure-cognitiveservices-spellcheck": "^1.0.1",
"botbuilder": "^4.1.3",
"ms-rest-azure": "^2.5.9",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are ms-rest-azure and restify dependencies for this library? I doesn't look like they are used in the middleware.

Copy link
Contributor

Choose a reason for hiding this comment

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

The test bot in the test folder is using restify. Could probably make this a dev dependency. My next package is going to implement the test adapter and mocha for tests, so we can use that as a template for unit tests moving forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of moving restify do devDepencies, my advise would be to create a samples folder like the dotNet repo has. And keep the test folder purely for unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once I put in TestAdapter unit testing along with mocha, we can follow that pattern for tests, and, yes, these test bots can then become samples.

szul added a commit that referenced this pull request Aug 19, 2019
Merge pull request #94 from szul/middleware
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.

None yet

4 participants