-
Notifications
You must be signed in to change notification settings - Fork 198
Fix #518: Exclude generic function in TSX file #521
Fix #518: Exclude generic function in TSX file #521
Conversation
for no-function-expression rule
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.
Thanks for the PR! 🚀 A couple of notes - I think this is structurally going to need some work.
src/noFunctionExpressionRule.ts
Outdated
@@ -33,6 +33,12 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
} | |||
|
|||
class NoFunctionExpressionRuleWalker extends ErrorTolerantWalker { | |||
protected visitSourceFile(node: ts.SourceFile): void { | |||
if (/.*\.tsx/.test(node.fileName) === false) { |
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'll accidentally skip non-tsx files in the rare case of someone including .tsx
in their file name before the extension, such as the very odd foo.tsx.ts
. Let's switch it to just checking if the string ends with the .tsx
extension. endsWith
is supported in all Node versions we support here.
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.
Also, the issue's conclusion seemed to be that we should still complain on function
s in .tsx files and only skip complaining if they have a generic. I think you'll need to still visit those source files, then individually check function
s to see if they have a generic type.
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.
@JoshuaKGoldberg I checked other place which deals with .tsx
(AstUtils.ts/getLanguageVariant
, etc.), they still use /.*\.tsx/.test(node.fileName)
, is there any plan to replace them with endsWith
as you suggest ?
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.
😲 Good find, that's a bug! Filed #525.
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.
Glad to have this in, thanks!
Very cool, thanks a lot @br1anchen! |
Thanks for making
hacktoberfest
issue 👍Fixes #518