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

feat: Update README, tests, dependencies #179

Merged
merged 6 commits into from
May 6, 2019
Merged

Conversation

germanattanasio
Copy link
Contributor

@Naktibalda I did some updates to your code to update the tests to use typescript and cleanup extra code that we had for when we were using plain javascript.

  • The code is now in src and lib is ignore from the repository.
  • The npm package will have lib with the types and javascript but not src with the typescript.
  • simple-bot was updated
  • multi-bot was not updated as I ran out of time 😞
  • switched to jest and added codecov for code coverage
  • Removed dependencies that were not needed.

What I need.

  • Can you please test that it works as expected for you?
  • I tried updating the README but I would like your 👀 there too.

---- for semantic versioning ---

BREAKING CHANGE:

  • Botkit updated to 4.0.0 which means there are a lot of changes to use your bot with botkit now.
  • Only simple bot example was updated.

BREAKING CHANGE:
* Botkit updated to 4.0.0 which means there are a lot of changes to use your bot with botkit now.
* Only simple bot example was updated.
Copy link
Contributor

@Naktibalda Naktibalda left a comment

Choose a reason for hiding this comment

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

After installing this branch I got an empty lib dir with .gitkeep file in it.

README.md Outdated
```typescript
function checkBalance(context, callback) {
```js
const checkBalance = (context) => new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function would look better with async.

README.md Show resolved Hide resolved
README.md Outdated

They can be customized as follows:

```js
middleware.before = function(message, assistantPayload, callback) {
middleware.before = (message, assistantPayload) => new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

middleware.before = (message, assistantPayload) => async {
   // Code here gets executed before making the call to Assistant.
  return assistantPayload;
} 

"dotenv": "^7.0.0",
"botbuilder-adapter-slack": "^1.0.1",
"botkit": "^4.0.1",
"botkit-middleware-watson": "file:../../botkit-middleware-watson-2.0.0.tgz",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change version constraint to ^2.0.0.

@Naktibalda
Copy link
Contributor

Is ignoring compiled files a standard practice in node-sdk?
It makes it impossible to install this library from branch because I get no src/ and no lib/.

How am I supposed to test my changes now?
I would rather remove package-lock.json from git, because this is a library, so lock file doesn't matter.

src/index.ts Outdated
import deepMerge = require('deepmerge');
import {promisify} from 'util';
import { BotkitMessage } from 'botkit';

export interface MiddlewareConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

I just used this in my code and I think that it would be better to name it WatsonMiddlewareConfig.

@germanattanasio
Copy link
Contributor Author

@Naktibalda I added back the lib files so that you can test this from github.

@codecov-io
Copy link

codecov-io commented May 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (botkit-next@9ac00a3). Click here to learn what that means.
The diff coverage is 78.57%.

Impacted file tree graph

@@              Coverage Diff              @@
##             botkit-next    #179   +/-   ##
=============================================
  Coverage               ?   78.5%           
=============================================
  Files                  ?       2           
  Lines                  ?     107           
  Branches               ?      22           
=============================================
  Hits                   ?      84           
  Misses                 ?      19           
  Partials               ?       4
Impacted Files Coverage Δ
lib/utils.js 100% <ø> (ø)
lib/index.js 65.15% <78.57%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ac00a3...47c18b5. Read the comment docs.

Copy link
Contributor

@Naktibalda Naktibalda left a comment

Choose a reason for hiding this comment

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

Thanks, after changing sendToWatsonAsync to sendToWatson, my bot works successfuly with your branch.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@germanattanasio germanattanasio merged commit c43137d into botkit-next May 6, 2019
@germanattanasio germanattanasio deleted the use-async branch May 6, 2019 21:30
@germanattanasio germanattanasio restored the use-async branch May 7, 2019 21:13
@watson-github-bot
Copy link
Member

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants