-
Notifications
You must be signed in to change notification settings - Fork 889
Add use-default-type-parameter
rule
#2253
Add use-default-type-parameter
rule
#2253
Conversation
Here's how I think we should proceed with this:
|
Actually, now that we support specifying a typescript version for a test (#2323), does this still need to be targeted to |
@andy-hanson you're right; this can stay on |
Note: Needed to change typescript devDependency to |
bump @andy-hanson, I think we're unblocked here now |
Still need to finish the 2.3 upgrade first (#2544) |
b2959fb
to
61364b6
Compare
}; | ||
/* tslint:enable:object-literal-sort-keys */ | ||
|
||
public static FAILURE_STRING = "This is the default value for this type parameter."; |
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 should be more descriptive by telling the user what to do, i.e. "This is the default value for this type parameter, it can be omitted."
} | ||
} | ||
|
||
interface ArgsAndParams { |
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.
nit: let's move this higher up in the file since it's used above this line
ctx.addFailureAtNode(arg, Rule.FAILURE_STRING, fix()); | ||
} | ||
|
||
function fix(): Lint.Fix { |
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.
nit: name this createFix()
case ts.SyntaxKind.ClassDeclaration: | ||
case ts.SyntaxKind.ClassExpression: | ||
case ts.SyntaxKind.TypeAliasDeclaration: | ||
return (decl as ts.ClassLikeDeclaration | ts.TypeAliasDeclaration).typeParameters; |
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.
hmm... how about this
import { isClassLikeDeclaration, isTypeAliasDeclaration } from "tsutils";
for (const decl of sym.declarations) {
if (isClassLikeDeclaration(decl) || isTypeAliasDeclaration(decl)) {
return decl.typeParameters;
}
}
@@ -1,7 +1,4 @@ | |||
{ | |||
"linterOptions": { | |||
"typeCheck": true | |||
}, |
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.
this change is unrelated, please revert...
class D extends C<number> {} | ||
~~~~~~ [0] | ||
|
||
[0]: This is the default value for this type parameter. |
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.
can we add some tests for interfaces too?
PR checklist
Overview of change:
Added the
use-default-type-parameter
rule, which encourages the use of defaults for type parameters.Note: Tests will of course only work with
typescript@next
(to be[email protected]
when it releases). How should we handle this so that CircleCI doesn't fail? Maybe the TSLint testing tool could manage installations of TypeScript, and allow settings for rules that only work in newer versions?CHANGELOG.md entry:
[new-rule]
use-default-type-parameter