-
Notifications
You must be signed in to change notification settings - Fork 117
Conversation
statements: [50, 80] |
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.
Don't want to bump these watermarks up? I think we should aim for > 90% or >95% coverage on core modules.
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 do that in bulk for all repos once they are done.
@@ -1,12 +1,10 @@ | |||
.editorconfig | |||
.jshintrc |
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.
probably need .eslintrc.yml
in there instead of .jshintrc
@@ -1,107 +0,0 @@ | |||
// TODO: don't have the defs from the hooks PR yet, fix later |
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.
Is this moved to the https://github.com/feathersjs-ecosystem/feathers-typescript repo? Just curious.
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.
Yeah, will be. I don't want to mess with our repos because TypeScript definitions are broken all the time. They'll be published separately and then pulled in.
package.json
Outdated
"lint": "semistandard --fix", | ||
"mocha": "mocha --opts mocha.opts", | ||
"test": "npm run compile && npm run lint && npm run coverage && nsp check", | ||
"test": "npm run lint && npm run coverage", |
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.
Are we dropping the nsp
check? Does Greenkeeper cover us there?
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.
Nah, good call, added it back.
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.
A couple comments. Other than that, looks good.
For feathersjs/feathers#608