From 0ee9fa293a7f8b369394ead813bcbb2df87cc294 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Wed, 29 May 2019 12:48:44 +0700 Subject: [PATCH 01/28] Add noForIn rule --- src/rules/noForInRule.ts | 57 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 src/rules/noForInRule.ts diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts new file mode 100644 index 00000000000..062d3fffb33 --- /dev/null +++ b/src/rules/noForInRule.ts @@ -0,0 +1,57 @@ +/** + * @license + * Copyright 2013 Palantir Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { isForInStatement } from "tsutils"; +import * as ts from "typescript"; +import * as Lint from "../index"; + +export class Rule extends Lint.Rules.AbstractRule { + public static metadata: Lint.IRuleMetadata = { + ruleName: 'no-for-in', + description: "Recommended the avoidance of 'for-in' statements. They can be replaced by Object.keys in a 'for-of' loop.", + rationale: + "A for(... of ...) loop is easier to implement and read when a for(... in ...) loop, as for(... in ...) require a hasOwnProperty check on objects to ensure proper behaviour.", + optionsDescription: "Not configurable.", + options: null, + optionExamples: [true], + type: "typescript", + typescriptOnly: false, + }; + + public static FAILURE_STRING_FACTORY(initializer: string, expression: string): string { + //tslint:disable-next-line:max-line-length + return `Do not use the 'for in' statement: 'for (${initializer} in ${expression})'. If this is an object, use 'Object.keys' instead. If this is an array use a standard 'for' or 'for of' loop instead.`; + } + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithFunction(sourceFile, walk); + } +} + +function walk(ctx: Lint.WalkContext) { + function cb(node: ts.Node): void { + if (isForInStatement(node)) { + const initializer: string = node.initializer.getText(); + const expression: string = node.expression.getText(); + + const msg: string = Rule.FAILURE_STRING_FACTORY(initializer, expression); + ctx.addFailureAt(node.getStart(), node.getWidth(), msg); + } + return ts.forEachChild(node, cb); + } + + return ts.forEachChild(ctx.sourceFile, cb); +} \ No newline at end of file From b3c8c97ee460ac89f973e64e93ad82826a6c87b9 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Wed, 29 May 2019 14:20:51 +0700 Subject: [PATCH 02/28] tslint fixes --- src/rules/noForInRule.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index 062d3fffb33..a904c10f586 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -16,12 +16,14 @@ */ import { isForInStatement } from "tsutils"; import * as ts from "typescript"; + import * as Lint from "../index"; export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { - ruleName: 'no-for-in', - description: "Recommended the avoidance of 'for-in' statements. They can be replaced by Object.keys in a 'for-of' loop.", + description: + "Recommended the avoidance of 'for-in' statements. They can be replaced by Object.keys in a 'for-of' loop.", + ruleName: "no-for-in", rationale: "A for(... of ...) loop is easier to implement and read when a for(... in ...) loop, as for(... in ...) require a hasOwnProperty check on objects to ensure proper behaviour.", optionsDescription: "Not configurable.", @@ -32,7 +34,6 @@ export class Rule extends Lint.Rules.AbstractRule { }; public static FAILURE_STRING_FACTORY(initializer: string, expression: string): string { - //tslint:disable-next-line:max-line-length return `Do not use the 'for in' statement: 'for (${initializer} in ${expression})'. If this is an object, use 'Object.keys' instead. If this is an array use a standard 'for' or 'for of' loop instead.`; } @@ -41,7 +42,7 @@ export class Rule extends Lint.Rules.AbstractRule { } } -function walk(ctx: Lint.WalkContext) { +function walk(ctx: Lint.WalkContext) { function cb(node: ts.Node): void { if (isForInStatement(node)) { const initializer: string = node.initializer.getText(); @@ -54,4 +55,4 @@ function walk(ctx: Lint.WalkContext) { } return ts.forEachChild(ctx.sourceFile, cb); -} \ No newline at end of file +} From 4fd5d476d9fe6d2a8489fccf77c9ac4ca30c6967 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Wed, 29 May 2019 14:31:53 +0700 Subject: [PATCH 03/28] test fixes --- src/configs/all.ts | 1 + test/rules/no-for-in/test.ts.lint | 20 ++++++++++++++++++++ test/rules/no-for-in/tslint.json | 5 +++++ 3 files changed, 26 insertions(+) create mode 100644 test/rules/no-for-in/test.ts.lint create mode 100644 test/rules/no-for-in/tslint.json diff --git a/src/configs/all.ts b/src/configs/all.ts index fb98cc7f242..61b9b553a01 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -51,6 +51,7 @@ export const rules = { }, "no-any": true, "no-empty-interface": true, + "no-for-in": true, "no-import-side-effect": true, // Technically this is not the strictest setting, but don't want to conflict with "typedef" "no-inferrable-types": { options: ["ignore-params"] }, diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint new file mode 100644 index 00000000000..a8ed06827a5 --- /dev/null +++ b/test/rules/no-for-in/test.ts.lint @@ -0,0 +1,20 @@ +// this should pass +const columns = [{ data: [1] }]; +for (let i = 0; i < columns[0].data.length; i++) { + columns.map((x) => x.data[i]); +} + +// this should NOT pass +const object = {test:1, test2:1 ,test3:1}; +for (const key in object) { + ~~~~~~~~~~~~~~~~~~~~~~~~~ [Expected a 'for-of' loop instead of a 'for-in' loop.] + if (object.hasOwnProperty(key)) { + const element = object[key]; + + } +} + +// this should pass +for (const key of Object.keys(object)) { + const value = object[key]; +} \ No newline at end of file diff --git a/test/rules/no-for-in/tslint.json b/test/rules/no-for-in/tslint.json new file mode 100644 index 00000000000..2a62503d3e3 --- /dev/null +++ b/test/rules/no-for-in/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "no-for-in": true + } +} From 3fc8769402b57529a4a65f0beffce1ae42cf202a Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Wed, 29 May 2019 14:41:08 +0700 Subject: [PATCH 04/28] test fix --- src/rules/noForInRule.ts | 6 +++--- test/rules/no-for-in/test.ts.lint | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index a904c10f586..100e2836563 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -23,12 +23,12 @@ export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { description: "Recommended the avoidance of 'for-in' statements. They can be replaced by Object.keys in a 'for-of' loop.", - ruleName: "no-for-in", - rationale: - "A for(... of ...) loop is easier to implement and read when a for(... in ...) loop, as for(... in ...) require a hasOwnProperty check on objects to ensure proper behaviour.", optionsDescription: "Not configurable.", options: null, optionExamples: [true], + rationale: + "A for(... of ...) loop is easier to implement and read when a for(... in ...) loop, as for(... in ...) require a hasOwnProperty check on objects to ensure proper behaviour.", + ruleName: "no-for-in", type: "typescript", typescriptOnly: false, }; diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint index a8ed06827a5..b381bc74085 100644 --- a/test/rules/no-for-in/test.ts.lint +++ b/test/rules/no-for-in/test.ts.lint @@ -7,7 +7,7 @@ for (let i = 0; i < columns[0].data.length; i++) { // this should NOT pass const object = {test:1, test2:1 ,test3:1}; for (const key in object) { - ~~~~~~~~~~~~~~~~~~~~~~~~~ [Expected a 'for-of' loop instead of a 'for-in' loop.] +~~~~~~~~~~~~~~~~~~~~~~~~~ [Expected a 'for-of' loop instead of a 'for-in' loop.] if (object.hasOwnProperty(key)) { const element = object[key]; From d23b9865b0e772a1be1d2b02a14be4d49bdf64b9 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 7 Jun 2019 17:03:07 +0700 Subject: [PATCH 05/28] Update noForInRule.ts --- src/rules/noForInRule.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index 100e2836563..b62c5541f98 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -23,8 +23,8 @@ export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { description: "Recommended the avoidance of 'for-in' statements. They can be replaced by Object.keys in a 'for-of' loop.", - optionsDescription: "Not configurable.", options: null, + optionsDescription: "Not configurable.", optionExamples: [true], rationale: "A for(... of ...) loop is easier to implement and read when a for(... in ...) loop, as for(... in ...) require a hasOwnProperty check on objects to ensure proper behaviour.", From ae05ee67e085c59742fd7b973f0252fddd21888d Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 7 Jun 2019 17:12:59 +0700 Subject: [PATCH 06/28] fixed test errors --- src/configuration.ts | 36 +++++++++++-------------- src/rules/completed-docs/exclusions.ts | 12 ++++----- src/rules/noForInRule.ts | 2 +- src/rules/noImplicitDependenciesRule.ts | 3 +++ 4 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/configuration.ts b/src/configuration.ts index baf9d59bf0c..6d9ff15a557 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -311,8 +311,8 @@ function resolveConfigurationPath(filePath: string, relativeTo?: string) { } catch (err) { throw new Error( `Invalid "extends" configuration value - could not require "${filePath}". ` + - "Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) " + - "for the approximate method TSLint uses to find the referenced configuration file.", + "Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) " + + "for the approximate method TSLint uses to find the referenced configuration file.", ); } } @@ -328,12 +328,10 @@ export function extendConfigurationFile( add(nextProperty); return combinedProperty as T; - function add(property: T | undefined): void { + function add(property: any): void { if (property !== undefined) { - for (const name in property) { - if (hasOwnProperty(property, name)) { - combinedProperty[name] = property[name]; - } + for (const name of Object.keys(property) as string[]) { + combinedProperty[name] = property[name]; } } } @@ -517,9 +515,9 @@ export type RawRuleConfig = | boolean | any[] | { - severity?: RuleSeverity | "warn" | "none" | "default"; - options?: any; - }; + severity?: RuleSeverity | "warn" | "none" | "default"; + options?: any; + }; /** * Parses a config file and normalizes legacy config settings. @@ -578,10 +576,8 @@ export function parseConfigFile( function parseRules(config: RawRulesConfig | undefined): Map> { const map = new Map>(); if (config !== undefined) { - for (const ruleName in config) { - if (hasOwnProperty(config, ruleName)) { - map.set(ruleName, parseRuleOptions(config[ruleName], defaultSeverity)); - } + for (const ruleName of Object.keys(config)) { + map.set(ruleName, parseRuleOptions(config[ruleName], defaultSeverity)); } } return map; @@ -618,15 +614,15 @@ export function parseConfigFile( return { ...(raw.exclude !== undefined ? { - exclude: arrayify(raw.exclude).map(pattern => - dir === undefined ? path.resolve(pattern) : path.resolve(dir, pattern), - ), - } + exclude: arrayify(raw.exclude).map(pattern => + dir === undefined ? path.resolve(pattern) : path.resolve(dir, pattern), + ), + } : {}), ...(raw.format !== undefined ? { - format: raw.format, - } + format: raw.format, + } : {}), }; } diff --git a/src/rules/completed-docs/exclusions.ts b/src/rules/completed-docs/exclusions.ts index f575fa2df9c..69982e1843c 100644 --- a/src/rules/completed-docs/exclusions.ts +++ b/src/rules/completed-docs/exclusions.ts @@ -49,13 +49,11 @@ const addRequirements = (exclusionsMap: ExclusionsMap, descriptors: IInputExclus return; } - for (const docType in descriptors) { - if (hasOwnProperty(descriptors, docType)) { - exclusionsMap.set( - docType as DocType, - createRequirementsForDocType(docType as DocType, descriptors[docType]), - ); - } + for (const docType of Object.keys(descriptors)) { + exclusionsMap.set( + docType as DocType, + createRequirementsForDocType(docType as DocType, descriptors[docType]), + ); } }; diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index b62c5541f98..b8d9b0db2dc 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -23,9 +23,9 @@ export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { description: "Recommended the avoidance of 'for-in' statements. They can be replaced by Object.keys in a 'for-of' loop.", + optionExamples: [true], options: null, optionsDescription: "Not configurable.", - optionExamples: [true], rationale: "A for(... of ...) loop is easier to implement and read when a for(... in ...) loop, as for(... in ...) require a hasOwnProperty check on objects to ensure proper behaviour.", ruleName: "no-for-in", diff --git a/src/rules/noImplicitDependenciesRule.ts b/src/rules/noImplicitDependenciesRule.ts index 681f910365f..9bbeaaad632 100644 --- a/src/rules/noImplicitDependenciesRule.ts +++ b/src/rules/noImplicitDependenciesRule.ts @@ -165,6 +165,9 @@ function getDependencies(fileName: string, options: Options): Set { } function addDependencies(result: Set, dependencies: Dependencies) { + for (const name of Object.keys(dependencies)) { + result.add(name); + } for (const name in dependencies) { if (dependencies.hasOwnProperty(name)) { result.add(name); From d977b3c84b9a66ad52ebd20e2dc4d6e3b9badd85 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 7 Jun 2019 17:14:29 +0700 Subject: [PATCH 07/28] Update exclusions.ts --- src/rules/completed-docs/exclusions.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rules/completed-docs/exclusions.ts b/src/rules/completed-docs/exclusions.ts index 69982e1843c..e6746628174 100644 --- a/src/rules/completed-docs/exclusions.ts +++ b/src/rules/completed-docs/exclusions.ts @@ -15,7 +15,6 @@ * limitations under the License. */ -import { hasOwnProperty } from "../../utils"; import { DESCRIPTOR_OVERLOADS, DocType } from "../completedDocsRule"; import { BlockExclusion, IBlockExclusionDescriptor } from "./blockExclusion"; From 28038f9c16e2895a380848724547a0b4fc4f68f5 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 7 Jun 2019 17:31:49 +0700 Subject: [PATCH 08/28] Update configuration.ts --- src/configuration.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/configuration.ts b/src/configuration.ts index 6d9ff15a557..3f7354e159b 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -24,7 +24,7 @@ import * as path from "path"; import { FatalError, showWarningOnce } from "./error"; import { IOptions, RuleSeverity } from "./language/rule/rule"; import { findRule } from "./ruleLoader"; -import { arrayify, hasOwnProperty, stripComments, tryResolvePackage } from "./utils"; +import { arrayify, stripComments, tryResolvePackage } from "./utils"; export interface IConfigurationFile { /** From 2edf878b4037fe4b352f4f6e89b535c42d3b37ba Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Sun, 9 Jun 2019 12:14:55 +0700 Subject: [PATCH 09/28] fix tests --- src/configs/all.ts | 6 +----- src/configuration.ts | 34 +++++++++++++++++++--------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/configs/all.ts b/src/configs/all.ts index 61b9b553a01..5ecdc6a290b 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -297,11 +297,7 @@ export const RULES_EXCLUDED_FROM_ALL_CONFIG = [ // Exclude typescript-only rules from jsRules, otherwise it's identical. export const jsRules: { [key: string]: any } = {}; -for (const key in rules) { - if (!hasOwnProperty(rules, key)) { - continue; - } - +for (const key of Object.keys(rules)) { const Rule = findRule(key, joinPaths(__dirname, "..", "rules")); if (Rule === undefined) { throw new Error(`Couldn't find rule '${key}'.`); diff --git a/src/configuration.ts b/src/configuration.ts index 3f7354e159b..521bf32ab4f 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -311,26 +311,30 @@ function resolveConfigurationPath(filePath: string, relativeTo?: string) { } catch (err) { throw new Error( `Invalid "extends" configuration value - could not require "${filePath}". ` + - "Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) " + - "for the approximate method TSLint uses to find the referenced configuration file.", + "Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) " + + "for the approximate method TSLint uses to find the referenced configuration file.", ); } } +type keyValue = { + [s: string]: any; +}; + export function extendConfigurationFile( targetConfig: IConfigurationFile, nextConfigSource: IConfigurationFile, ): IConfigurationFile { - function combineProperties(targetProperty: T | undefined, nextProperty: T | undefined): T { + function combineProperties(targetProperty: keyValue | undefined, nextProperty: keyValue | undefined): keyValue { const combinedProperty: { [key: string]: any } = {}; add(targetProperty); // next config source overwrites the target config object add(nextProperty); - return combinedProperty as T; + return combinedProperty; - function add(property: any): void { + function add(property: keyValue | undefined): void { if (property !== undefined) { - for (const name of Object.keys(property) as string[]) { + for (const name of Object.keys(property)) { combinedProperty[name] = property[name]; } } @@ -515,9 +519,9 @@ export type RawRuleConfig = | boolean | any[] | { - severity?: RuleSeverity | "warn" | "none" | "default"; - options?: any; - }; + severity?: RuleSeverity | "warn" | "none" | "default"; + options?: any; + }; /** * Parses a config file and normalizes legacy config settings. @@ -614,15 +618,15 @@ export function parseConfigFile( return { ...(raw.exclude !== undefined ? { - exclude: arrayify(raw.exclude).map(pattern => - dir === undefined ? path.resolve(pattern) : path.resolve(dir, pattern), - ), - } + exclude: arrayify(raw.exclude).map(pattern => + dir === undefined ? path.resolve(pattern) : path.resolve(dir, pattern), + ), + } : {}), ...(raw.format !== undefined ? { - format: raw.format, - } + format: raw.format, + } : {}), }; } From 9a2a5a2a061863b7edec6b3e2b3db8e2153bb94f Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Sun, 9 Jun 2019 12:16:28 +0700 Subject: [PATCH 10/28] FIX TESTS --- src/configs/all.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/configs/all.ts b/src/configs/all.ts index 5ecdc6a290b..2420b02bfff 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -18,7 +18,6 @@ import { join as joinPaths } from "path"; import { findRule } from "../ruleLoader"; -import { hasOwnProperty } from "../utils"; // tslint:disable object-literal-sort-keys // tslint:disable object-literal-key-quotes From 7dfd006310d1b9489c858124fce2809c8c6f7971 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Sun, 9 Jun 2019 12:19:32 +0700 Subject: [PATCH 11/28] Update test.ts.lint --- test/rules/no-for-in/test.ts.lint | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint index b381bc74085..eed99ce285a 100644 --- a/test/rules/no-for-in/test.ts.lint +++ b/test/rules/no-for-in/test.ts.lint @@ -7,12 +7,15 @@ for (let i = 0; i < columns[0].data.length; i++) { // this should NOT pass const object = {test:1, test2:1 ,test3:1}; for (const key in object) { -~~~~~~~~~~~~~~~~~~~~~~~~~ [Expected a 'for-of' loop instead of a 'for-in' loop.] - if (object.hasOwnProperty(key)) { +~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Expected a 'for-of' loop instead of a 'for-in' loop.] + if (object.hasOwnProperty(key)) {. +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ const element = object[key]; - +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ } +~~ } +~ // this should pass for (const key of Object.keys(object)) { From 2c4d18434935b93a575cffd2c574d2f72698afc5 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Sun, 9 Jun 2019 12:24:42 +0700 Subject: [PATCH 12/28] Update test.ts.lint --- test/rules/no-for-in/test.ts.lint | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint index eed99ce285a..e9a74d50e7b 100644 --- a/test/rules/no-for-in/test.ts.lint +++ b/test/rules/no-for-in/test.ts.lint @@ -7,15 +7,15 @@ for (let i = 0; i < columns[0].data.length; i++) { // this should NOT pass const object = {test:1, test2:1 ,test3:1}; for (const key in object) { -~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Expected a 'for-of' loop instead of a 'for-in' loop.] +~~~~~~~~~~~~~~~~~~~~~~~~~~~ if (object.hasOwnProperty(key)) {. -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ const element = object[key]; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ } ~~ } -~ +~ [Do not use the 'for in' statement: 'for (const key in object)'. If this is an object, use 'Object.keys' instead. If this is an array use a standard 'for' or 'for of' loop instead.] // this should pass for (const key of Object.keys(object)) { From 9a39d3927237d3609da40f11d3f1d921965ddb65 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Sun, 9 Jun 2019 12:30:19 +0700 Subject: [PATCH 13/28] fix tests --- src/configuration.ts | 11 +++++++---- src/rules/noImplicitDependenciesRule.ts | 5 ----- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/configuration.ts b/src/configuration.ts index 521bf32ab4f..a2cbb1532d4 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -317,22 +317,25 @@ function resolveConfigurationPath(filePath: string, relativeTo?: string) { } } -type keyValue = { +interface KeyValue { [s: string]: any; -}; +} export function extendConfigurationFile( targetConfig: IConfigurationFile, nextConfigSource: IConfigurationFile, ): IConfigurationFile { - function combineProperties(targetProperty: keyValue | undefined, nextProperty: keyValue | undefined): keyValue { + function combineProperties( + targetProperty: KeyValue | undefined, + nextProperty: KeyValue | undefined, + ): KeyValue { const combinedProperty: { [key: string]: any } = {}; add(targetProperty); // next config source overwrites the target config object add(nextProperty); return combinedProperty; - function add(property: keyValue | undefined): void { + function add(property: KeyValue | undefined): void { if (property !== undefined) { for (const name of Object.keys(property)) { combinedProperty[name] = property[name]; diff --git a/src/rules/noImplicitDependenciesRule.ts b/src/rules/noImplicitDependenciesRule.ts index 9bbeaaad632..1a124dddf17 100644 --- a/src/rules/noImplicitDependenciesRule.ts +++ b/src/rules/noImplicitDependenciesRule.ts @@ -168,11 +168,6 @@ function addDependencies(result: Set, dependencies: Dependencies) { for (const name of Object.keys(dependencies)) { result.add(name); } - for (const name in dependencies) { - if (dependencies.hasOwnProperty(name)) { - result.add(name); - } - } } function findPackageJson(current: string): string | undefined { From d1544fb8d6bfb9f83a0ab9488d34bb196c09485a Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 28 Jun 2019 16:30:36 +0800 Subject: [PATCH 14/28] Update src/rules/noForInRule.ts Co-Authored-By: Adi Dahiya --- src/rules/noForInRule.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index b8d9b0db2dc..6fbf2e5e286 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -34,7 +34,7 @@ export class Rule extends Lint.Rules.AbstractRule { }; public static FAILURE_STRING_FACTORY(initializer: string, expression: string): string { - return `Do not use the 'for in' statement: 'for (${initializer} in ${expression})'. If this is an object, use 'Object.keys' instead. If this is an array use a standard 'for' or 'for of' loop instead.`; + return `Use a for...of statement instead of for...in. If iterating over an object, use Object.keys() to access its enumerable keys.`; } public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { From 7fc2632a0e8159374df89a89106ecd92bda763f1 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 28 Jun 2019 16:31:11 +0800 Subject: [PATCH 15/28] Update test/rules/no-for-in/test.ts.lint --- test/rules/no-for-in/test.ts.lint | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint index e9a74d50e7b..c27840b7a27 100644 --- a/test/rules/no-for-in/test.ts.lint +++ b/test/rules/no-for-in/test.ts.lint @@ -8,7 +8,7 @@ for (let i = 0; i < columns[0].data.length; i++) { const object = {test:1, test2:1 ,test3:1}; for (const key in object) { ~~~~~~~~~~~~~~~~~~~~~~~~~~~ - if (object.hasOwnProperty(key)) {. + if (object.hasOwnProperty(key)) { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ const element = object[key]; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -20,4 +20,4 @@ for (const key in object) { // this should pass for (const key of Object.keys(object)) { const value = object[key]; -} \ No newline at end of file +} From 165609914c9952336470388a6f119f49cc3bbd8a Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 28 Jun 2019 16:31:33 +0800 Subject: [PATCH 16/28] Update test/rules/no-for-in/test.ts.lint --- test/rules/no-for-in/test.ts.lint | 1 - 1 file changed, 1 deletion(-) diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint index c27840b7a27..e9b74988462 100644 --- a/test/rules/no-for-in/test.ts.lint +++ b/test/rules/no-for-in/test.ts.lint @@ -1,4 +1,3 @@ -// this should pass const columns = [{ data: [1] }]; for (let i = 0; i < columns[0].data.length; i++) { columns.map((x) => x.data[i]); From 0e089b8bf551e5fa8fd447f143f3c86153999a3c Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 28 Jun 2019 16:32:07 +0800 Subject: [PATCH 17/28] Update test/rules/no-for-in/test.ts.lint --- test/rules/no-for-in/test.ts.lint | 1 - 1 file changed, 1 deletion(-) diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint index e9b74988462..22596e88d97 100644 --- a/test/rules/no-for-in/test.ts.lint +++ b/test/rules/no-for-in/test.ts.lint @@ -16,7 +16,6 @@ for (const key in object) { } ~ [Do not use the 'for in' statement: 'for (const key in object)'. If this is an object, use 'Object.keys' instead. If this is an array use a standard 'for' or 'for of' loop instead.] -// this should pass for (const key of Object.keys(object)) { const value = object[key]; } From 0da8c12f64435214b379528310f79d413dfb55a0 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 28 Jun 2019 16:33:31 +0800 Subject: [PATCH 18/28] Update test/rules/no-for-in/test.ts.lint --- test/rules/no-for-in/test.ts.lint | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint index 22596e88d97..69513263e18 100644 --- a/test/rules/no-for-in/test.ts.lint +++ b/test/rules/no-for-in/test.ts.lint @@ -4,7 +4,7 @@ for (let i = 0; i < columns[0].data.length; i++) { } // this should NOT pass -const object = {test:1, test2:1 ,test3:1}; +const object = { test: 1, test2: 1, test3: 1 }; for (const key in object) { ~~~~~~~~~~~~~~~~~~~~~~~~~~~ if (object.hasOwnProperty(key)) { From 1b6090d0f48521213eebffc4e2c3802d8604daeb Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 28 Jun 2019 16:33:52 +0800 Subject: [PATCH 19/28] Update test/rules/no-for-in/test.ts.lint --- test/rules/no-for-in/test.ts.lint | 1 - 1 file changed, 1 deletion(-) diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint index 69513263e18..f6d196498e7 100644 --- a/test/rules/no-for-in/test.ts.lint +++ b/test/rules/no-for-in/test.ts.lint @@ -3,7 +3,6 @@ for (let i = 0; i < columns[0].data.length; i++) { columns.map((x) => x.data[i]); } -// this should NOT pass const object = { test: 1, test2: 1, test3: 1 }; for (const key in object) { ~~~~~~~~~~~~~~~~~~~~~~~~~~~ From 635cdb4c6042b9e16d004848cae0f2d0dea6624b Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 28 Jun 2019 16:38:18 +0800 Subject: [PATCH 20/28] Update noForInRule.ts --- src/rules/noForInRule.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index 6fbf2e5e286..3ef1b608152 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -33,7 +33,7 @@ export class Rule extends Lint.Rules.AbstractRule { typescriptOnly: false, }; - public static FAILURE_STRING_FACTORY(initializer: string, expression: string): string { + public static FAILURE_STRING_FACTORY(_: string, __: string): string { return `Use a for...of statement instead of for...in. If iterating over an object, use Object.keys() to access its enumerable keys.`; } From 18a362783b256765fb2121aeab5d230d9a3f94bc Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Fri, 28 Jun 2019 16:41:05 +0800 Subject: [PATCH 21/28] Update test.ts.lint --- test/rules/no-for-in/test.ts.lint | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint index f6d196498e7..a3dd806a466 100644 --- a/test/rules/no-for-in/test.ts.lint +++ b/test/rules/no-for-in/test.ts.lint @@ -13,7 +13,7 @@ for (const key in object) { } ~~ } -~ [Do not use the 'for in' statement: 'for (const key in object)'. If this is an object, use 'Object.keys' instead. If this is an array use a standard 'for' or 'for of' loop instead.] +~ [Use a for...of statement instead of for...in. If iterating over an object, use Object.keys() to access its enumerable keys.] for (const key of Object.keys(object)) { const value = object[key]; From 7142921a43e6c1353257be21fd2d6dd2a738a5b2 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Sat, 29 Jun 2019 12:58:48 +0800 Subject: [PATCH 22/28] Update src/rules/noForInRule.ts Co-Authored-By: Adi Dahiya --- src/rules/noForInRule.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index 3ef1b608152..8975bf9b584 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -33,7 +33,7 @@ export class Rule extends Lint.Rules.AbstractRule { typescriptOnly: false, }; - public static FAILURE_STRING_FACTORY(_: string, __: string): string { + public static FAILURE_STRING_FACTORY(): string { return `Use a for...of statement instead of for...in. If iterating over an object, use Object.keys() to access its enumerable keys.`; } From ba6c7b43541d48339e9818d64d119bcfc113d750 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Sat, 29 Jun 2019 12:58:58 +0800 Subject: [PATCH 23/28] Update src/rules/noForInRule.ts Co-Authored-By: Adi Dahiya --- src/rules/noForInRule.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index 8975bf9b584..b91445f51b5 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -48,7 +48,7 @@ function walk(ctx: Lint.WalkContext) { const initializer: string = node.initializer.getText(); const expression: string = node.expression.getText(); - const msg: string = Rule.FAILURE_STRING_FACTORY(initializer, expression); + const msg: string = Rule.FAILURE_STRING_FACTORY(); ctx.addFailureAt(node.getStart(), node.getWidth(), msg); } return ts.forEachChild(node, cb); From 1024247cba605ceb13844363a0dc69297a79e0de Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Sat, 29 Jun 2019 13:11:06 +0800 Subject: [PATCH 24/28] Update noForInRule.ts --- src/rules/noForInRule.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index b91445f51b5..d523ca47053 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -45,9 +45,6 @@ export class Rule extends Lint.Rules.AbstractRule { function walk(ctx: Lint.WalkContext) { function cb(node: ts.Node): void { if (isForInStatement(node)) { - const initializer: string = node.initializer.getText(); - const expression: string = node.expression.getText(); - const msg: string = Rule.FAILURE_STRING_FACTORY(); ctx.addFailureAt(node.getStart(), node.getWidth(), msg); } From b79168b161400d5498ade4f799db82b888783de0 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Wed, 3 Jul 2019 13:22:07 +0800 Subject: [PATCH 25/28] Apply suggestions from code review Co-Authored-By: Adi Dahiya --- src/configuration.ts | 12 ++++++------ src/rules/noForInRule.ts | 7 ++++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/configuration.ts b/src/configuration.ts index a2cbb1532d4..46cced074e5 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -325,17 +325,17 @@ export function extendConfigurationFile( targetConfig: IConfigurationFile, nextConfigSource: IConfigurationFile, ): IConfigurationFile { - function combineProperties( - targetProperty: KeyValue | undefined, - nextProperty: KeyValue | undefined, - ): KeyValue { + function combineProperties( + targetProperty: T | undefined, + nextProperty: T | undefined, + ): T { const combinedProperty: { [key: string]: any } = {}; add(targetProperty); // next config source overwrites the target config object add(nextProperty); - return combinedProperty; + return combinedProperty as T; - function add(property: KeyValue | undefined): void { + function add(property: T | undefined): void { if (property !== undefined) { for (const name of Object.keys(property)) { combinedProperty[name] = property[name]; diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index d523ca47053..a2c17305bc0 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2013 Palantir Technologies, Inc. + * Copyright 2019 Palantir Technologies, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + import { isForInStatement } from "tsutils"; import * as ts from "typescript"; @@ -22,12 +23,12 @@ import * as Lint from "../index"; export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { description: - "Recommended the avoidance of 'for-in' statements. They can be replaced by Object.keys in a 'for-of' loop.", + "Ban the usage of for...in statements.", optionExamples: [true], options: null, optionsDescription: "Not configurable.", rationale: - "A for(... of ...) loop is easier to implement and read when a for(... in ...) loop, as for(... in ...) require a hasOwnProperty check on objects to ensure proper behaviour.", + "for...in statements are legacy JavaScript syntax which usually require a verbose `hasOwnProperty` check inside their loop body. These statements can be fully replaced with for...of statements in modern JS & TS.", ruleName: "no-for-in", type: "typescript", typescriptOnly: false, From 825f63fe1240ef747df7fcd81e081034be8fa8f6 Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Wed, 3 Jul 2019 13:33:38 +0800 Subject: [PATCH 26/28] Update configuration.ts --- src/configuration.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/configuration.ts b/src/configuration.ts index 46cced074e5..d5e3a60c873 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -317,10 +317,6 @@ function resolveConfigurationPath(filePath: string, relativeTo?: string) { } } -interface KeyValue { - [s: string]: any; -} - export function extendConfigurationFile( targetConfig: IConfigurationFile, nextConfigSource: IConfigurationFile, From 6da67d8f95f09124f220fdc242221c09be42789d Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Thu, 4 Jul 2019 09:32:14 +0800 Subject: [PATCH 27/28] Update src/rules/noForInRule.ts Co-Authored-By: Adi Dahiya --- src/rules/noForInRule.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index a2c17305bc0..ae4af641cb0 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -22,7 +22,7 @@ import * as Lint from "../index"; export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { - description: + description: "Ban the usage of for...in statements.", "Ban the usage of for...in statements.", optionExamples: [true], options: null, From b0d00ea995a91151a0198c3b99b395436275b59b Mon Sep 17 00:00:00 2001 From: Josh Pike Date: Thu, 4 Jul 2019 09:32:30 +0800 Subject: [PATCH 28/28] Update src/rules/noForInRule.ts Co-Authored-By: Adi Dahiya --- src/rules/noForInRule.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts index ae4af641cb0..4a485c45b55 100644 --- a/src/rules/noForInRule.ts +++ b/src/rules/noForInRule.ts @@ -23,7 +23,6 @@ import * as Lint from "../index"; export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { description: "Ban the usage of for...in statements.", - "Ban the usage of for...in statements.", optionExamples: [true], options: null, optionsDescription: "Not configurable.",