-
Notifications
You must be signed in to change notification settings - Fork 890
Allow regex for requires #3516
Allow regex for requires #3516
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.
LGTM with suggestions
src/rules/noRequireImportsRule.ts
Outdated
@@ -37,12 +51,17 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
public static FAILURE_STRING = "require() style import is forbidden"; | |||
|
|||
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | |||
return this.applyWithFunction(sourceFile, walk); | |||
const patternConfig = this.ruleArguments[this.ruleArguments.length - 1] as { "ignore-module": string } | undefined; | |||
const ignorePattern = patternConfig === undefined ? undefined : new RegExp(patternConfig[OPTION_IGNORE_MODULE]); |
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 check if "ignore-module"
property is not undefined
src/rules/noVarRequiresRule.ts
Outdated
const options = this.getOptions() as any[]; | ||
const patternConfig = options[0] as { "ignore-module": string } | undefined; | ||
const ignorePattern = patternConfig === undefined ? undefined : new RegExp(patternConfig[OPTION_IGNORE_MODULE]); | ||
|
||
if (this.getCurrentDepth() <= 1 && expression.kind === ts.SyntaxKind.Identifier) { | ||
const identifierName = (expression as ts.Identifier).text; | ||
if (identifierName === "require") { |
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.
you can add an additional condition to ensure there is only one argument. that way you don't have to loop through the arguments below
src/rules/noVarRequiresRule.ts
Outdated
@@ -52,11 +66,19 @@ class NoVarRequiresWalker extends Lint.ScopeAwareRuleWalker<{}> { | |||
public visitCallExpression(node: ts.CallExpression) { | |||
const expression = node.expression; | |||
|
|||
const options = this.getOptions() as any[]; | |||
const patternConfig = options[0] as { "ignore-module": string } | undefined; | |||
const ignorePattern = patternConfig === undefined ? undefined : new RegExp(patternConfig[OPTION_IGNORE_MODULE]); |
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.
consider moving this block of code right before the value is actually used to avoid unnecessary work
src/rules/noRequireImportsRule.ts
Outdated
@@ -37,12 +51,17 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
public static FAILURE_STRING = "require() style import is forbidden"; | |||
|
|||
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | |||
return this.applyWithFunction(sourceFile, walk); | |||
const patternConfig = this.ruleArguments[this.ruleArguments.length - 1] as { "ignore-module": string } | undefined; |
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.
why not simply this.ruleArguments[0]
?
src/rules/noRequireImportsRule.ts
Outdated
minLength: 0, | ||
type: "array", | ||
}, | ||
optionExamples: [true, [true, { [OPTION_IGNORE_MODULE]: "(\\.html|\\.css)$" }]], |
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 would actually prefer this to be a list of regex patterns. One big pattern like in no-import-side-effect
is very hard to maintain.
#3504 also uses multiple patterns.
options: { | ||
items: { | ||
properties: { | ||
"ignore-module": { |
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.
Shouldn't this be [OPTION_IGNORE_MODULE]
instead of ignore-module
?
options: { | ||
items: { | ||
properties: { | ||
"ignore-module": { |
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 [OPTION_IGNORE_MODULE]
instead of ignore-module
?
@@ -20,15 +20,33 @@ import * as ts from "typescript"; | |||
|
|||
import * as Lint from "../index"; | |||
|
|||
const OPTION_IGNORE_MODULE = "ignore-module"; | |||
|
|||
export class Rule extends Lint.Rules.AbstractRule { | |||
/* tslint:disable:object-literal-sort-keys */ | |||
public static metadata: Lint.IRuleMetadata = { | |||
ruleName: "no-require-imports", | |||
description: "Disallows invocation of `require()`.", | |||
rationale: "Prefer the newer ES6-style imports over `require()`.", | |||
optionsDescription: "Not configurable.", |
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 don't know how this property is used but with the new option the rule is in fact configurable, no?
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.
@helenanderson please update this branch so we can review it, or close it if no longer relevant. we will close this if we do not hear from you in two weeks.
function walk(ctx: Lint.WalkContext<void>) { | ||
for (const name of findImports(ctx.sourceFile, ImportKind.AllRequireLike)) { | ||
ctx.addFailureAtNode(name.parent!, Rule.FAILURE_STRING); | ||
function extractIgnoreModule(patternConfig: { "ignore-module": string[] } | undefined) { |
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 pass the string[] | undefined
instead of the whole object type
minLength: 0, | ||
type: "array", | ||
}, | ||
optionExamples: [true, [true, { [OPTION_IGNORE_MODULE]: ["\\.html$", "\\.css$"] }]], |
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 is a bad example because html and CSS files are not appropriate for TSLint.
} | ||
} | ||
|
||
super.visitCallExpression(node); | ||
} | ||
} | ||
|
||
function extractIgnoreModule(patternConfig: { "ignore-module": string[] } | undefined) { |
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.
again, unwrap the options at a higher level
Closing due to age and inactivity. Feel free to re-open or create a new pull request if you decide to continue working on this. |
PR checklist
Overview of change:
Modifies noVarRequiresRule and noRequireImportsRule with an option to specify a regex for which to ignore the rule. Modeled after the ignore-module in noImportSideEffectRule.
Is there anything you'd like reviewers to focus on?
CHANGELOG.md entry: