-
Notifications
You must be signed in to change notification settings - Fork 14
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
chore(dep)!: upgrade gts 2.0.0 #194
Conversation
Codecov Report
@@ Coverage Diff @@
## master #194 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 2 2
Lines 371 371
Branches 45 42 -3
=====================================
Hits 371 371
Continue to review full report at Codecov.
|
package.json
Outdated
"codecov": "^3.0.4", | ||
"gts": "^1.0.0", | ||
"eslint": "^6.8.0", |
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.
We can drop all of these eslint
dependencies
package.json
Outdated
"prepare": "npm run compile", | ||
"pretest": "npm run compile", | ||
"posttest": "npm run lint", | ||
"docs": "compodoc src/", | ||
"presystem-test": "npm run compile", | ||
"samples-test": "cd samples/ && npm link ../ && npm test && cd ../", | ||
"samples-test": "cd samples/ && npm link ../ && npm install && npm test && cd ../", |
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.
@bcoe do we do this in other places?
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.
all that should be required is this:
"samples-test": "cd samples/ && npm link ../ && npm test && cd ../"
which is what we use in other libraries, npm link ../
should perform the install.
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.
FYI: The couple libraries I converted use samples-test": "cd samples/ && npm link ../ && npm install && npm test && cd ../
, like https://github.com/googleapis/nodejs-monitoring/blob/master/package.json#L32
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, left a note that we should probably come back and flesh out these types eventually.
@@ -47,7 +47,7 @@ export class ResourceStream<T> extends Transform implements ResourceEvents<T> { | |||
this._requestsMade = 0; | |||
this._resultsToSend = args.maxResults === -1 ? Infinity : args.maxResults!; | |||
} | |||
// tslint:disable-next-line:no-any | |||
/* eslint-disable @typescript-eslint/no-explicit-any */ |
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.
I think this is fine for now, I would rather that these PRs turn into as minimal of a refactor as possible, even though we might want to come back and make this a more extensive refactor.
upgrade gts 2.0.0
fix some lint issue.