Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Commit

Permalink
Throw error if rules directory supplied does not exist
Browse files Browse the repository at this point in the history
If rules directory is supplied in a tslint.json or package.json file,
look for directory relative to .json file.

If rules directory is supplied via CLI,
look for directory relative to CwD.
  • Loading branch information
Jason Killian committed Jan 15, 2016
1 parent fac7c9a commit a57e61e
Show file tree
Hide file tree
Showing 7 changed files with 123 additions and 38 deletions.
72 changes: 48 additions & 24 deletions src/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand All @@ -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}`);
}
}

Expand Down
20 changes: 13 additions & 7 deletions src/tslint-cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 ...]")
Expand Down Expand Up @@ -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);
Expand Down
20 changes: 19 additions & 1 deletion test/check-bin.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
6 changes: 6 additions & 0 deletions test/config/tslint-custom-rules-with-dir.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"rulesDirectory": "../files/custom-rules/",
"rules": {
"always-fail": true
}
}
5 changes: 5 additions & 0 deletions test/config/tslint-custom-rules.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"always-fail": true
}
}
27 changes: 27 additions & 0 deletions test/files/custom-rules/alwaysFailRule.js
Original file line number Diff line number Diff line change
@@ -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");

This comment has been minimized.

Copy link
@alexeagle

alexeagle Jul 19, 2016

Contributor

is it intentional that this test asserts against the released version of tslint in node_modules rather than the built version (eg using require("../../../bin/tslint/lib/lint") ) ??
It seems like this test only observes failures following a release, rather than catching issues introduced locally.

cc @ScottSWu

This comment has been minimized.

Copy link
@jkillian

jkillian Jul 20, 2016

Contributor

Well, this is just an arbitrary custom rule the tests load (it's not designed to test anything specifically in and of itself); the actual tests will still be running using the built version of TSLint.

However, it does seem to make more sense to require the build version of TSLint, and possibly that could help catch possible bugs, so I'm fine with this changing 👍

This comment has been minimized.

Copy link
@ScottSWu

ScottSWu Jul 20, 2016

Contributor

Quick reproduction: /ScottSWu/tslint/tree/tslint_custom_rules

check-bin.sh fails on failure.getStatementsCount is not a function because the custom rules create RuleFailure from tslint:latest while proseFormatter uses RuleFailure from the recently built tslint. Changing it to ../../../lib/lint works.

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);
11 changes: 5 additions & 6 deletions test/ruleLoaderTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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} = {
Expand All @@ -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);
});

Expand All @@ -39,7 +38,7 @@ describe("Rule Loader", () => {
"invalidConfig2": false
};

const rules = loadRules(invalidConfiguration, {}, rulesDirectory);
const rules = loadRules(invalidConfiguration, {}, RULES_DIRECTORY);
assert.deepEqual(rules, []);
});

Expand All @@ -51,7 +50,7 @@ describe("Rule Loader", () => {
"eofline-": true
};

const rules = loadRules(invalidConfiguration, {}, rulesDirectory);
const rules = loadRules(invalidConfiguration, {}, RULES_DIRECTORY);
assert.deepEqual(rules, []);
});

Expand All @@ -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);
});
});

0 comments on commit a57e61e

Please sign in to comment.