-
Notifications
You must be signed in to change notification settings - Fork 281
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
Update LUIS to new (Azure) nodejs SDK #766
Conversation
…n Autorest) which impacted tests
Pull Request Test Coverage Report for Build #1996
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pick between async-file
or fs-extra
in the project but not both.
libraries/botbuilder-ai/package.json
Outdated
"botbuilder": "~4.1.6", | ||
"build": "^0.1.4", | ||
"fs-extra": "^7.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that including fs-extra
will introduce 2 dependencies that do the same thing into our production code. async-file
is used currently and it makes sense to pick one and standardize on it. fs-extra
is my first choice but async-file
is fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Justin for the review! I'm not sure where you're seeing the dependency on async-file
. fs-extra
appears to be being used by the bot framework, so we should probably keep that.
The new set of bits is adoption of new ms-rest stuff which doesn't appear to have a async-file dependency (?).
It looks like the unit tests were updated but coveralls is reporting the entire error block missing coverage. Any ideas what happened here? |
Nice catch, I checked in fixed tests! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I was also the last person to touch this PR 😄 @justinwilaby, @daveta or @cleemullins can you take a look?
All feedback addressed, looking to merge.
In conjunction with microsoft/botbuilder-dotnet#1367.