Skip to content

fix: update dependencies and examples #1036

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
Merged
merged 8 commits into from
May 5, 2020
Merged

fix: update dependencies and examples #1036

merged 8 commits into from
May 5, 2020

Conversation

germanattanasio
Copy link
Contributor

Updated a few dependencies and based on the CHANGELOGs. Dependencies are dropping Node 4 and 6 support. async moved to 3.0 but no changes for us.

I disable a breaking change in prettier and disable rules in the generated unit tests.

@germanattanasio germanattanasio requested a review from dpopp07 March 30, 2020 16:49
dpopp07
dpopp07 approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@dpopp07
Copy link
Contributor

dpopp07 commented Mar 30, 2020

Looks like integration tests are failing on a bad assert. Something must have changed in the service instance.

@germanattanasio germanattanasio requested a review from dpopp07 April 30, 2020 15:32
@lgtm-com
Copy link

lgtm-com bot commented Apr 30, 2020

This pull request introduces 2 alerts and fixes 1 when merging 3f0c2cb into c6953c0 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

fixed alerts:

  • 1 for Syntax error

@codecov-io
Copy link

codecov-io commented Apr 30, 2020 •
edited
Loading

Codecov Report

Merging #1036 into master will increase coverage by 11.11%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1036       +/-   ##
===========================================
+ Coverage   77.77%   88.88%   +11.11%     
===========================================
  Files           1        1               
  Lines           9        9               
  Branches        2        2               
===========================================
+ Hits            7        8        +1     
+ Misses          2        1        -1
Impacted Files Coverage Δ
test/resources/auth_helper.js 88.88% <0.00%> (+11.11%) ⬆️

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 c6953c0...3f0c2cb. Read the comment docs.

dpopp07
dpopp07 requested changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

I appreciate all of the style fixes in here but I think there are two main subjects to address still:

  1. The latest version uses authenticator instead of iam_apikey for authentication.
  2. The top-level params are now lower camel case instead of snake case

I commented on two of these instances but not all of them

examples/.eslintrc.js Outdated
'no-process-exit': 'off',
'prefer-const': 'off',
'no-var': 'off',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess our lint config should follow the lint rules, huh? 👍

examples/assistant.v1.js Outdated
password: process.env.ASSISTANT_PASSWORD || '<assistant_password>',
version: '2018-02-16'
// See: https://github.com/watson-developer-cloud/node-sdk#authentication
// iam_apikey: 'INSERT YOUR IAM API KEY HERE',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for the latest version, this should be authenticator and not iam_apikey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this mean we need to add an authentication field?

examples/assistant_tone_analyzer_integration/tone_assistant_integration.v1.js Outdated
@@ -66,8 +49,8 @@ var maintainToneHistoryInContext = true;
var payload = {
workspace_id: process.env.WORKSPACE_ID || '<workspace_id>',
Copy link
Contributor

Choose a reason for hiding this comment

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

The keys are now camel case: workspaceId

@lgtm-com
Copy link

lgtm-com bot commented May 5, 2020

This pull request fixes 1 alert when merging df4e02d into c6953c0 - view on LGTM.com

fixed alerts:

  • 1 for Syntax error

dpopp07
dpopp07 approved these changes May 5, 2020
View reviewed changes
Copy link
Contributor

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for doing this @germanattanasio

@germanattanasio germanattanasio changed the title build: update dependencies fix: update dependencies and examples May 5, 2020
@germanattanasio germanattanasio merged commit fde46cf into master May 5, 2020
@germanattanasio germanattanasio deleted the update_dependencies branch May 5, 2020 16:12
watson-github-bot pushed a commit that referenced this pull request Jun 3, 2020
@semantic-release-bot
chore(release): 5.6.0 [skip ci]
d07bd84 
# [5.6.0](v5.5.0...v5.6.0) (2020-06-03)

### Bug Fixes

* update dependencies and examples ([#1036](#1036)) ([fde46cf](fde46cf))

### Features

* **assistant-v2:** new method: `messageStateless` ([0ccc5bf](0ccc5bf))
* **visual-recognition-v4:** new method: `getModelFile` ([55aec8c](55aec8c))
@watson-github-bot
Copy link
Member

🎉 This PR is included in version 5.6.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
Reviewers

@dpopp07 dpopp07 dpopp07 approved these changes

Assignees
No one assigned
Labels
released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@germanattanasio @dpopp07 @codecov-io @watson-github-bot