From 4b911106ba4efa13996f05ada4a2ec7450330bc1 Mon Sep 17 00:00:00 2001 From: Noam Yogev Date: Fri, 9 Nov 2018 19:45:02 +0200 Subject: [PATCH] #147 implement use-simple-attribute (#628) * implement use-simple-attribute * fix typo in useSimpleAttributeRule reorder use-simple-attribute in tslint.json * add rationale to useSimpleAttributeRule * Merge branch master; rename to plural rule * Added description to README.md --- README.md | 10 +++ recommended_ruleset.js | 1 + src/tests/useSimpleAttributeRuleTests.ts | 102 +++++++++++++++++++++++ src/useSimpleAttributesRule.ts | 96 +++++++++++++++++++++ tslint-warnings.csv | 1 + tslint.json | 1 + 6 files changed, 211 insertions(+) create mode 100644 src/tests/useSimpleAttributeRuleTests.ts create mode 100644 src/useSimpleAttributesRule.ts diff --git a/README.md b/README.md index e1c9e73fc..edc4f2b15 100644 --- a/README.md +++ b/README.md @@ -934,6 +934,16 @@ We recommend you specify exact versions of lint libraries, including `tslint-mic 6.0.0-beta + + + use-simple-attributes + + + Simpler attributes in JSX and TSX files helps keep code clean and readable. + Separate complex expressions into their own line and use clear variable names to make your code more understandable. + + 6.0.0 + react-a11y-props diff --git a/recommended_ruleset.js b/recommended_ruleset.js index 4a1bb1cb2..07f7bf61e 100644 --- a/recommended_ruleset.js +++ b/recommended_ruleset.js @@ -96,6 +96,7 @@ module.exports = { 'triple-equals': [true, 'allow-null-check'], 'use-isnan': true, 'use-named-parameter': true, + 'use-simple-attributes': true, /** * Code Clarity. The following rules should be turned on because they make the code diff --git a/src/tests/useSimpleAttributeRuleTests.ts b/src/tests/useSimpleAttributeRuleTests.ts new file mode 100644 index 000000000..69b33328a --- /dev/null +++ b/src/tests/useSimpleAttributeRuleTests.ts @@ -0,0 +1,102 @@ +import { Utils } from '../utils/Utils'; +import { TestHelper } from './TestHelper'; +/** + * Unit tests. + */ +describe('useSimpleAttributesRule', (): void => { + const ruleName: string = 'use-simple-attributes'; + const binaryExpressionErrorMessage: string = 'Attribute contains a complex binary expression'; + const ternaryExpressionErrorMessage: string = 'Attribute contains a ternary expression'; + it('should fail if only attribute initializer is a complex binary expression', (): void => { + const script: string = ` + import React = require('react'); + const element = \`; + `; + + TestHelper.assertViolations(ruleName, script, [ + { + failure: binaryExpressionErrorMessage, + name: Utils.absolutePath('file.tsx'), + ruleName: ruleName, + startPosition: { character: 25, line: 3 } + } + ]); + }); + it('should fail if any attribute initializer is a complex binary expression', (): void => { + const script: string = ` + import React = require('react'); + const element = \`; + `; + + TestHelper.assertViolations(ruleName, script, [ + { + failure: binaryExpressionErrorMessage, + name: Utils.absolutePath('file.tsx'), + ruleName: ruleName, + startPosition: { character: 25, line: 3 } + } + ]); + }); + it('should pass if any attribute initializer is a simple binary expression', (): void => { + const script: string = ` + import React = require('react'); + const element = \`; + `; + + TestHelper.assertNoViolation(ruleName, script); + }); + it('should fail if only attribute initializer is a ternary expression', (): void => { + const script: string = ` + import React = require('react'); + const someVar = 3; + const element = \`; + `; + + TestHelper.assertViolations(ruleName, script, [ + { + failure: ternaryExpressionErrorMessage, + name: Utils.absolutePath('file.tsx'), + ruleName: ruleName, + startPosition: { character: 25, line: 4 } + } + ]); + }); + it('should fail if any attribute initializer is a ternary expression', (): void => { + const script: string = ` + import React = require('react'); + const someVar = 3; + const element = \`; + `; + + TestHelper.assertViolations(ruleName, script, [ + { + failure: ternaryExpressionErrorMessage, + name: Utils.absolutePath('file.tsx'), + ruleName: ruleName, + startPosition: { character: 25, line: 4 } + } + ]); + }); + it('should fail if any attribute initializer is a ternary expression or a complex binary expression', (): void => { + const script: string = ` + import React = require('react'); + const someVar = 3; + const element = \`; + `; + + TestHelper.assertViolations(ruleName, script, [ + { + failure: ternaryExpressionErrorMessage, + name: Utils.absolutePath('file.tsx'), + ruleName: ruleName, + startPosition: { character: 25, line: 4 } + }, + { + failure: binaryExpressionErrorMessage, + name: Utils.absolutePath('file.tsx'), + ruleName: ruleName, + startPosition: { character: 25, line: 4 } + } + ]); + }); +}); diff --git a/src/useSimpleAttributesRule.ts b/src/useSimpleAttributesRule.ts new file mode 100644 index 000000000..2adc4eb10 --- /dev/null +++ b/src/useSimpleAttributesRule.ts @@ -0,0 +1,96 @@ +import * as ts from 'typescript'; +import * as Lint from 'tslint'; + +import { ExtendedMetadata } from './utils/ExtendedMetadata'; +import { getJsxAttributesFromJsxElement } from './utils/JsxAttribute'; + +export class Rule extends Lint.Rules.AbstractRule { + public static metadata: ExtendedMetadata = { + ruleName: 'use-simple-attributes', + type: 'functionality', + description: 'Enforce usage of only simple attribute types.', + rationale: + 'Simpler attributes in JSX and TSX files helps keep code clean and readable.\ + Separate complex expressions into their own line and use clear variable names to make your code more understandable.', + options: null, // tslint:disable-line:no-null-keyword + optionsDescription: '', + typescriptOnly: false, + issueClass: 'Non-SDL', + issueType: 'Error', + severity: 'Important', + level: 'Opportunity for Excellence', + group: 'Correctness' + }; + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return sourceFile.languageVariant === ts.LanguageVariant.JSX + ? this.applyWithWalker(new UseSimpleAttributesRuleWalker(sourceFile, this.getOptions())) + : []; + } +} + +class UseSimpleAttributesRuleWalker extends Lint.RuleWalker { + protected visitJsxSelfClosingElement(node: ts.JsxSelfClosingElement): void { + this.checkJsxOpeningElement(node); + super.visitJsxSelfClosingElement(node); + } + + protected visitJsxElement(node: ts.JsxElement): void { + this.checkJsxOpeningElement(node.openingElement); + super.visitJsxElement(node); + } + + private checkJsxOpeningElement(node: ts.JsxOpeningLikeElement) { + const attributes = getJsxAttributesFromJsxElement(node); + for (const key of Object.keys(attributes)) { + const attribute = attributes[key]; + + // Handle Binary Expressions + const binaryExpression = this.getNextNodeRecursive(attribute, ts.SyntaxKind.BinaryExpression); + if (binaryExpression && !this.isSimpleBinaryExpression(binaryExpression)) { + const binaryExpressionErrorMessage: string = 'Attribute contains a complex binary expression'; + this.addFailureAt(node.getStart(), node.getWidth(), binaryExpressionErrorMessage); + } + + // Handle Ternary Expression + const ternaryExpression = this.getNextNodeRecursive(attribute, ts.SyntaxKind.ConditionalExpression); + if (ternaryExpression) { + const ternaryExpressionErrorMessage: string = 'Attribute contains a ternary expression'; + this.addFailureAt(node.getStart(), node.getWidth(), ternaryExpressionErrorMessage); + } + } + } + + private isSimpleBinaryExpression(binaryExpression: ts.BinaryExpression): boolean { + if (binaryExpression.kind !== ts.SyntaxKind.BinaryExpression) { + return false; + } + + // Both children of a Binary Expression should be primitives, constants or identifiers + const allowedBinaryNodes: ts.SyntaxKind[] = [ + ts.SyntaxKind.NumericLiteral, + ts.SyntaxKind.StringLiteral, + ts.SyntaxKind.TrueKeyword, + ts.SyntaxKind.FalseKeyword, + ts.SyntaxKind.Identifier + ]; + + const leftTerm = allowedBinaryNodes.find(kind => kind === binaryExpression.left.kind); + const rightTerm = allowedBinaryNodes.find(kind => kind === binaryExpression.right.kind); + return leftTerm ? (rightTerm ? true : false) : false; + } + + private getNextNodeRecursive(node: ts.Node, kind: ts.SyntaxKind): ts.Node | undefined { + if (!node) { + return undefined; + } + const childNodes = node.getChildren(); + let match = childNodes.find(cn => cn.kind === kind); + if (!match) { + for (const childNode of childNodes) { + match = this.getNextNodeRecursive(childNode, kind); + } + } + return match; + } +} diff --git a/tslint-warnings.csv b/tslint-warnings.csv index 3ffb68d9a..efa1804a1 100644 --- a/tslint-warnings.csv +++ b/tslint-warnings.csv @@ -310,6 +310,7 @@ unified-signatures,Warns for any two overloads that could be unified into one by use-default-type-parameter,Warns if an explicitly specified type argument is the default for that type parameter.,TSLINTLMNGTP,tslint,Non-SDL,Warning,Low,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,710,"CWE 710 - Coding Standards Violation" use-isnan,Enforces use of the `isNaN()` function to check for NaN references instead of a comparison to the `NaN` constant.,TSLINTPUV7LC,tslint,Non-SDL,Error,Critical,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,398,"CWE 398 - Indicator of Poor Code Quality" use-named-parameter,"Do not reference the arguments object by numerical index; instead, use a named parameter.",TSLINTKPEHQG,tslint,Non-SDL,Warning,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,710,"CWE 710 - Coding Standards Violation" +use-simple-attributes,Enforce usage of only simple attribute types.,TSLINT1PG0L9J,tslint,Non-SDL,Error,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,, valid-typeof,Ensures that the results of typeof are compared against a valid string.,TSLINT1IB59P1,tslint,Non-SDL,Error,Critical,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,, variable-name,Checks variable names for various errors.,TSLINT1CIV7K3,tslint,Non-SDL,Warning,Important,Opportunity for Excellence,See description on the tslint or tslint-microsoft-contrib website,TSLint Procedure,"398, 710","CWE 398 - Indicator of Poor Code Quality CWE 710 - Coding Standards Violation" diff --git a/tslint.json b/tslint.json index d382ca878..c8ed7ae45 100644 --- a/tslint.json +++ b/tslint.json @@ -155,6 +155,7 @@ "underscore-consistent-invocation": true, "use-default-type-parameter": false, "use-named-parameter": true, + "use-simple-attributes": true, // tslint-microsoft-contrib rules disabled "missing-jsdoc": false,