-
Notifications
You must be signed in to change notification settings - Fork 887
Conversation
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.
SGTM with comments
src/rules/completedDocsRule.ts
Outdated
@@ -389,7 +389,8 @@ class CompletedDocsWalker extends Lint.ProgramAwareRuleWalker { | |||
} | |||
|
|||
private checkNode(node: ts.Declaration, nodeType: DocType): void { | |||
if (node.name === undefined) { | |||
const { name } = node as ts.NamedDeclaration; |
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.
just update the type annotation of the parameter
package.json
Outdated
@@ -72,7 +72,7 @@ | |||
"rimraf": "^2.5.4", | |||
"tslint": "latest", | |||
"tslint-test-config-non-relative": "file:test/external/tslint-test-config-non-relative", | |||
"typescript": "^2.3.0" | |||
"typescript": "next" |
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.
not sure if it makes sense to use typescript@next in development while having a yarn.lock. I guess there'll be many merge conflicts just because of the typescript version in yarn.lock
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 agree, I don't want this change in development
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 won't compile in typescript pre-next
though. Could I just tag it to the specific daily version?
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.
would that mean that tslint API consumers would need to upgrade to 2.4.0-dev as well? I'm very wary of making this change; even if the compiled tslint typings continue to work with 2.3 in this PR, bumping to a nightly version opens up the door to future API breaks
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.
@nchen63 has another approach to fixing master that compiles with ts 2.3: https://github.com/palantir/tslint/tree/fix-master
@andy-hanson I released |
PR checklist
Overview of change:
TypeScript has made some breaking changes recently (microsoft/TypeScript#15486, microsoft/TypeScript#15594, microsoft/TypeScript#15887), so tslint must be updated.
Waiting on ajafff/tsutils#3.