-
Notifications
You must be signed in to change notification settings - Fork 889
add rule to detect throwing of string literals #1878
Conversation
Thanks for your interest in palantir/tslint, @atscott! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
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 you enable CircleCI on your fork?
@@ -0,0 +1,23 @@ | |||
let a = () => throw 'bla'; |
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.
add tests for template string and template strings with replacement variables
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.
Enabled CircleCI, added tests and logic for handling template strings, and fixed errors from 'npm run verify'
export class Rule extends Lint.Rules.AbstractRule { | ||
/* tslint:disable:object-literal-sort-keys */ | ||
public static metadata: Lint.IRuleMetadata = { | ||
ruleName: "string-throws", |
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 you rename to no-string-throw
to be consistent with other names?
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.
note, non-plural
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.
Done.
} | ||
|
||
class Walker extends Lint.RuleWalker { | ||
public visitThrowStatement(node: ts.ThrowStatement) { |
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.
indent width should be 4 spaces
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.
Done.
`because only Errors produce proper stack traces.`, | ||
options: null, | ||
optionsDescription: "", | ||
type: "typescript", |
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.
type should be functionality
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.
Done.
8bd8b84
to
76d55d4
Compare
236e284
to
51e6b80
Compare
I messed up my branch pretty badly in a bad merge and rebase squash. I think I got everything back to normal and addressed your comments. |
@atscott thanks! |
Wouldn't it be better to have a type-based rule to check that anything thrown is an |
I kind of went back and forth on this one. Yes, type checking would catch all cases and be simpler to implement, but it also requires |
I generally agree that lightweight syntactic checks that cover the majority of real-world use cases are useful to have in the core ruleset. |
PR checklist
What changes did you make?
I've added a rule that detects when a throw statement has a string literal, which does not produce a proper stack trace. The suggested fix is to change
throw 'string'
tothrow new Error('string')
. I didn't see an existing issue for this, but can create one if needed.