Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Fix #518: Exclude generic function in TSX file #521

Merged

Conversation

br1anchen
Copy link
Contributor

Thanks for making hacktoberfest issue 👍
Fixes #518

@msftclas
Copy link

msftclas commented Oct 6, 2018

CLA assistant check
All CLA requirements met.

Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a 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.

@@ -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) {

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.

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 functions 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 functions to see if they have a generic type.

Copy link
Contributor Author

@br1anchen br1anchen Oct 7, 2018

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 ?

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.

Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a 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!

@JoshuaKGoldberg JoshuaKGoldberg merged commit 7574010 into microsoft:master Oct 7, 2018
@astorije
Copy link
Contributor

astorije commented Oct 8, 2018

Very cool, thanks a lot @br1anchen!

@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0-beta0 milestone Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants