Skip to content

fix: update dependencies and examples#1036

Merged
germanattanasio merged 8 commits intomasterfrom
update_dependencies
May 5, 2020
Merged

fix: update dependencies and examples#1036
germanattanasio merged 8 commits intomasterfrom
update_dependencies

Conversation

@germanattanasio
Copy link
Copy Markdown
Contributor

@germanattanasio germanattanasio commented Mar 30, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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 suggested changes Apr 30, 2020
View reviewed changes
Copy link
Copy Markdown
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

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

@dpopp07 dpopp07 Apr 30, 2020

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? 👍

Comment thread 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
Copy Markdown
Contributor

@dpopp07 dpopp07 Apr 30, 2020

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
Copy Markdown
Contributor Author

@germanattanasio germanattanasio Apr 30, 2020

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?

Comment thread 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
Copy Markdown
Contributor

@dpopp07 dpopp07 Apr 30, 2020

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
Copy Markdown

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
Copy Markdown
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

Comment thread examples/browserify/.env.example
@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
Copy Markdown
Collaborator

watson-github-bot commented Jun 3, 2020

🎉 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

1 more reviewer

@dpopp07 dpopp07 dpopp07 approved these changes

Reviewers whose approvals may not affect merge requirements

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