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.

Sign up for GitHub

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

Jump to bottom

Cleanup javascript_nodejs/49.qnamaker-all-features sample #2986

Merged
merged 3 commits into from
Dec 15, 2020
Merged

Cleanup javascript_nodejs/49.qnamaker-all-features sample #2986

merged 3 commits into from
Dec 15, 2020

Conversation

stevengum
Copy link
Member

Related to #2959

The primary goal of this PR is cleanup in sample 49.qnamaker-all-features for Node.js so that developers have an easier "Getting started" experience.

5c5f453 focuses on cleaning up duplicate constant values.
e306ca2 contains additional cleanup to remove QnAMakerBaseDialog.

Proposed Changes

  • Remove qnamakerBaseDialog.js
    • Deletes subclass of botbuilder-ai's QnAMakerDialog in QnAMakerBaseDialog
  • Remove unused qnaCardBuilder.js
  • Cleanup README.md
  • Cleanup index.js
  • Trim copyright headers to match 2020 guidance

@HesselWellema FYI

@stevengum
cleanup default params in sample 49
5c5f453 
* for javascript_nodejs
@stevengum
directly use SDK's QnAMakerDialog
e306ca2 
* unify sample headers
* cleanup additional unnecessary code
@stevengum
Merge branch 'main' into stgum/2959
b5e9f50
@stevengum stevengum requested a review from a team as a code owner December 14, 2020 19:05
@stevengum
Copy link
Member Author

@sahithikkss as I mentioned offline, if you could review this PR when you have a chance it would be much appreciated 😄

sahithikkss-zz
sahithikkss-zz approved these changes Dec 15, 2020
View reviewed changes
Copy link
Contributor

@sahithikkss-zz sahithikkss-zz left a comment

Choose a reason for hiding this comment

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

Thanks for the effort to make get-started experience better..!
Tested and verified. Looks good to me.

@stevengum
Copy link
Member Author

Thanks for testing and approving the PR @sahithikkss & @joshgummersall!

@stevengum stevengum merged commit 7dbebc2 into main Dec 15, 2020
@stevengum stevengum deleted the stgum/2959 branch December 15, 2020 19:05
@stevengum stevengum mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sahithikkss-zz sahithikkss-zz sahithikkss-zz approved these changes

@joshgummersall joshgummersall joshgummersall approved these changes

@EricDahlvang EricDahlvang Awaiting requested review from EricDahlvang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stevengum @joshgummersall @sahithikkss-zz