Merge pull request #912 from palantir/rules-directory-fix

Improve handling of paths to custom rules directories

src/configuration.ts

@@ -51,39 +51,56 @@ export const DEFAULT_CONFIG = {
         ],
     }
 };
-const moduleDirectory = path.dirname(module.filename);
 
 export function findConfiguration(configFile: string, inputFileLocation: string): any {
-    if (configFile == null) {
-        // first look for package.json from input file location
-        configFile = findup("package.json", { cwd: inputFileLocation, nocase: true });
+    const configPath = findConfigurationPath(configFile, inputFileLocation);
+    return loadConfigurationFromPath(configPath);
+}
 
-        if (configFile) {
-            const content = require(configFile);
+export function findConfigurationPath(suppliedConfigFilePath: string, inputFilePath: string) {
+    if (suppliedConfigFilePath != null) {
+        if (!fs.existsSync(suppliedConfigFilePath)) {
+            throw new Error(`Could not find config file at: ${path.resolve(suppliedConfigFilePath)}`);
+        } else {
+            return suppliedConfigFilePath;
+        }
+    } else {
+        // search for package.json with tslintConfig property
+        let configFilePath = findup("package.json", { cwd: inputFilePath, nocase: true });
+        if (configFilePath != null && require(configFilePath).tslintConfig != null) {
+            return configFilePath;
+        }
 
-            if (content.tslintConfig) {
-                return content.tslintConfig;
-            }
+        // search for tslint.json from input file location
+        configFilePath = findup(CONFIG_FILENAME, { cwd: inputFilePath, nocase: true });
+        if (configFilePath != null && fs.existsSync(configFilePath)) {
+            return configFilePath;
         }
 
-        // next look for tslint.json
+        // search for tslint.json in home directory
         const homeDir = getHomeDir();
-        if (!homeDir) {
-            return undefined;
+        if (homeDir != null) {
+            configFilePath = path.join(homeDir, CONFIG_FILENAME);
+            if (fs.existsSync(configFilePath)) {
+                return configFilePath;
+            }
         }
 
-        const defaultPath = path.join(homeDir, CONFIG_FILENAME);
-
-        configFile = findup(CONFIG_FILENAME, { cwd: inputFileLocation, nocase: true }) || defaultPath;
+        // no path could be found
+        return undefined;
     }
+}
 
-    if (fs.existsSync(configFile)) {
-        let fileData = fs.readFileSync(configFile, "utf8");
+export function loadConfigurationFromPath(configFilePath: string) {
+    if (configFilePath == null) {
+        return DEFAULT_CONFIG;
+    } else if (path.basename(configFilePath) === "package.json") {
+        return require(configFilePath).tslintConfig;
+    } else {
+        let fileData = fs.readFileSync(configFilePath, "utf8");
         // remove BOM if present
         fileData = fileData.replace(/^\uFEFF/, "");
         return JSON.parse(fileData);
-    } else {
-        return DEFAULT_CONFIG;
     }
 }
 
@@ -103,20 +120,27 @@ function getHomeDir() {
     }
 }
 
-export function getRelativePath(directory: string): string {
+export function getRelativePath(directory: string, relativeTo?: string): string {
     if (directory != null) {
-        return path.relative(moduleDirectory, directory);
+        const basePath = relativeTo || process.cwd();
+        return path.resolve(basePath, directory);
     }
 }
 
-export function getRulesDirectories(directories: string | string[]): string[] {
+export function getRulesDirectories(directories: string | string[], relativeTo?: string): string[] {
     let rulesDirectories: string[] = [];
 
     if (directories != null) {
         if (typeof directories === "string") {
-            rulesDirectories = [getRelativePath(<string>directories)];
+            rulesDirectories = [getRelativePath(directories, relativeTo)];
         } else {
-            rulesDirectories = (<string[]>directories).map((dir) => getRelativePath(dir));
+            rulesDirectories = directories.map((dir) => getRelativePath(dir, relativeTo));
+        }
+    }
+
+    for (const directory of rulesDirectories) {
+        if (!fs.existsSync(directory)) {
+            throw new Error(`Could not find custom rule directory: ${directory}`);
         }
     }
 

src/tslint-cli.ts

@@ -18,8 +18,15 @@
 import * as fs from "fs";
 import * as glob from "glob";
 import * as optimist from "optimist";
+import * as path from "path";
 import * as Linter from "./tslint";
-import {CONFIG_FILENAME, DEFAULT_CONFIG, getRulesDirectories} from "./configuration";
+import {
+    CONFIG_FILENAME,
+    DEFAULT_CONFIG,
+    findConfigurationPath,
+    getRulesDirectories,
+    loadConfigurationFromPath
+} from "./configuration";
 
 let processed = optimist
     .usage("Usage: $0 [options] [file ...]")
@@ -167,14 +174,13 @@ const processFile = (file: string) => {
     }
 
     const contents = fs.readFileSync(file, "utf8");
-    const configuration = Linter.findConfiguration(argv.c, file);
+    const configurationPath = findConfigurationPath(argv.c, file);
+    const configuration = loadConfigurationFromPath(configurationPath);
 
-    if (configuration == null) {
-        console.error("Unable to find tslint configuration");
-        process.exit(1);
-    }
+    // if configurationPath is null, this will be set to ".", which is the current directory and is fine
+    const configurationDir = path.dirname(configurationPath);
 
-    const rulesDirectories = getRulesDirectories(configuration.rulesDirectory);
+    const rulesDirectories = getRulesDirectories(configuration.rulesDirectory, configurationDir);
 
     if (argv.r != null) {
         rulesDirectories.push(argv.r);

test/check-bin.sh

@@ -19,7 +19,10 @@ expectOut () {
   expect=$2
   msg=$3
 
-  if [ $expect != $actual ]; then
+  nodeV=`node -v`
+
+  # if Node 0.10.*, node will sometimes exit with status 8 when an error is thrown
+  if [[ $expect != $actual || $nodeV == v0.10.* && $expect == 1 && $actual == 8 ]] ; then
     echo "$msg: expected $expect got $actual"
     num_failures=$(expr $num_failures + 1)
   fi
@@ -42,6 +45,21 @@ expectOut $? 0 "tslint with valid arguments did not exit correctly"
 ./bin/tslint src/configuration.ts -f src/formatterLoader.ts
 expectOut $? 1 "tslint with -f flag did not exit correctly"
 
+# make sure calling tslint with a CLI custom rules directory that doesn't exist fails
+# (redirect stderr because it's confusing to see a stack trace during the build)
+./bin/tslint -c ./test/config/tslint-custom-rules.json -r ./someRandomDir src/tslint.ts
+expectOut $? 1 "tslint with -r pointing to a nonexistent directory did not fail"
+
+# make sure calling tslint with a CLI custom rules directory that does exist finds the errors it should
+./bin/tslint -c ./test/config/tslint-custom-rules.json -r ./test/files/custom-rules src/tslint.ts
+expectOut $? 2 "tslint with with -r pointing to custom rules did not find lint failures"
+
+# make sure calling tslint with a rulesDirectory in a config file works
+./bin/tslint -c ./test/config/tslint-custom-rules-with-dir.json src/tslint.ts
+expectOut $? 2 "tslint with with JSON pointing to custom rules did not find lint failures"
+
+
+
 # make sure tslint --init generates a file
 cd ./bin
 if [ -f tslint.json ]; then

test/config/tslint-custom-rules-with-dir.json

@@ -0,0 +1,6 @@
+{
+  "rulesDirectory": "../files/custom-rules/",
+  "rules": {
+    "always-fail": true
+  }
+}

test/config/tslint-custom-rules.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "always-fail": true
+  }
+}

test/files/custom-rules/alwaysFailRule.js

@@ -0,0 +1,27 @@
+var __extends = (this && this.__extends) || function (d, b) {
+    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
+    function __() { this.constructor = d; }
+    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
+};
+var Lint = require("tslint/lib/lint");
+var Rule = (function (_super) {
+    __extends(Rule, _super);
+    function Rule() {
+        _super.apply(this, arguments);
+    }
+    Rule.prototype.apply = function (sourceFile) {
+        return this.applyWithWalker(new AlwaysFailWalker(sourceFile, this.getOptions()));
+    };
+    return Rule;
+})(Lint.Rules.AbstractRule);
+exports.Rule = Rule;
+var AlwaysFailWalker = (function (_super) {
+    __extends(AlwaysFailWalker, _super);
+    function AlwaysFailWalker() {
+        _super.apply(this, arguments);
+    }
+    AlwaysFailWalker.prototype.visitSourceFile = function (node) {
+        this.addFailure(this.createFailure(node.getStart(), node.getWidth(), "failure"));
+    };
+    return AlwaysFailWalker;
+})(Lint.RuleWalker);

test/ruleLoaderTests.ts

@@ -17,8 +17,7 @@
 import {loadRules} from "./lint";
 
 describe("Rule Loader", () => {
-    const path = require("path");
-    const rulesDirectory = path.join(global.process.cwd(), "build/rules");
+    const RULES_DIRECTORY = "build/src/rules";
 
     it("loads core rules", () => {
         const validConfiguration: {[name: string]: any} = {
@@ -29,7 +28,7 @@ describe("Rule Loader", () => {
             "no-debugger": true
         };
 
-        const rules = loadRules(validConfiguration, {}, rulesDirectory);
+        const rules = loadRules(validConfiguration, {}, RULES_DIRECTORY);
         assert.equal(rules.length, 5);
     });
 
@@ -39,7 +38,7 @@ describe("Rule Loader", () => {
             "invalidConfig2": false
         };
 
-        const rules = loadRules(invalidConfiguration, {}, rulesDirectory);
+        const rules = loadRules(invalidConfiguration, {}, RULES_DIRECTORY);
         assert.deepEqual(rules, []);
     });
 
@@ -51,7 +50,7 @@ describe("Rule Loader", () => {
             "eofline-": true
         };
 
-        const rules = loadRules(invalidConfiguration, {}, rulesDirectory);
+        const rules = loadRules(invalidConfiguration, {}, RULES_DIRECTORY);
         assert.deepEqual(rules, []);
     });
 
@@ -64,7 +63,7 @@ describe("Rule Loader", () => {
             "no-debugger": true
         };
 
-        const rules = loadRules(validConfiguration, {}, [rulesDirectory]);
+        const rules = loadRules(validConfiguration, {}, [RULES_DIRECTORY]);
         assert.equal(rules.length, 5);
     });
 });