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

Override defaultSeverity defined in extended configs #3449

Merged
merged 8 commits into from
Jan 10, 2018
2 changes: 1 addition & 1 deletion docs/usage/configuration/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ A path to a directory or an array of paths to directories of [custom rules][2].
- Any rules specified in this block will override those configured in any base configuration being extended.
- [Check out the full rules list here][3].
* `jsRules?: any`: Same format as `rules`. These rules are applied to `.js` and `.jsx` files.
* `defaultSeverity?: "error" | "warning" | "off"`: The severity level used when a rule specifies a default warning level. If undefined, "error" is used. This value is not inherited and is only applied to rules in this file.
* `defaultSeverity?: "error" | "warning" | "off"`: The severity level used when a rule specifies a default warning level. If undefined, "error" is used. If specified, overrides the values defined in the configuration files which are extended by this configuration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you copy over the edit I suggested in configuration.ts to here? that would make it:

`defaultSeverity?: "error" | "warning" | "off"`: The severity level that is applied to rules in this config file as well as rules in any inherited config files which have their severity set to "default". If undefined, "error" is used as the defaultSeverity.

* `linterOptions?: { exclude?: string[] }`:
- `exclude: string[]`: An array of globs. Any file matching these globs will not be linted. All exclude patterns are relative to the configuration file they were specified in.

Expand Down
12 changes: 9 additions & 3 deletions src/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { arrayify, hasOwnProperty, stripComments } from "./utils";

export interface IConfigurationFile {
/**
* The severity that is applied to rules in _this_ config with `severity === "default"`.
* The severity that is applied to rules in this and _extended_ configs with `severity === "default"`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should probably be more verbose: The severity that is applied to rules in this config file as well as rules in any inherited config file which have their severity set to "default"

* Not inherited.
*/
defaultSeverity?: RuleSeverity;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this property declared on this interface? It's no longer of any use once the config is parsed. It's also never set by parseConfigFile and never read by anything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used on lines 70 and 78:

export const DEFAULT_CONFIG: IConfigurationFile = {
    defaultSeverity: "error",

export const EMPTY_CONFIG: IConfigurationFile = {
    defaultSeverity: "error",

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know. But the value is never read. In addition parseConfigFile and extendConfigurationFile simply don't set this property. That means you can basically never use it.
Since this could (very unlikely) break some users of the API, we should keep it around until the next major version. So this is not immediately actionable. I'll open a separate PR for that.

While you are already touching this code, can you please add @deprecated property is never set in the JSDoc comment? This could lead to lint errors on the two lines you mentioned above. You can just disable the error with // tslint:disable-line:deprecation.

Expand Down Expand Up @@ -200,9 +200,10 @@ function findup(filename: string, directory: string): string | undefined {
* 'path/to/config' will attempt to load a to/config file inside a node module named path
* @param configFilePath The configuration to load
* @param originalFilePath The entry point configuration file
* @param extendingConfig The configuration which extends this one
* @returns a configuration object for TSLint loaded from the file at configFilePath
*/
export function loadConfigurationFromPath(configFilePath?: string, originalFilePath = configFilePath) {
export function loadConfigurationFromPath(configFilePath?: string, originalFilePath = configFilePath, extendingConfig?: RawConfigFile) {
if (configFilePath == undefined) {
return DEFAULT_CONFIG;
} else {
Expand All @@ -224,14 +225,19 @@ export function loadConfigurationFromPath(configFilePath?: string, originalFileP
delete (require.cache as { [key: string]: any })[resolvedConfigFilePath];
}

// defaultSeverity defined in the config which extends this one wins.
if (extendingConfig !== undefined && extendingConfig.defaultSeverity !== undefined) {
rawConfigFile.defaultSeverity = extendingConfig.defaultSeverity;
}

const configFileDir = path.dirname(resolvedConfigFilePath);
const configFile = parseConfigFile(rawConfigFile, configFileDir);

// load configurations, in order, using their identifiers or relative paths
// apply the current configuration last by placing it last in this array
const configs: IConfigurationFile[] = configFile.extends.map((name) => {
const nextConfigFilePath = resolveConfigurationPath(name, configFileDir);
return loadConfigurationFromPath(nextConfigFilePath, originalFilePath);
return loadConfigurationFromPath(nextConfigFilePath, originalFilePath, rawConfigFile);
}).concat([configFile]);

return configs.reduce(extendConfigurationFile, EMPTY_CONFIG);
Expand Down
6 changes: 6 additions & 0 deletions test/config/tslint-default-severity-error.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"defaultSeverity": "error",
"rules": {
"default-severity-error": { "severity": "default" }
}
}
5 changes: 5 additions & 0 deletions test/config/tslint-default-severity-unspecified.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"default-severity-unspecified": { "severity": "default" }
}
}
7 changes: 7 additions & 0 deletions test/config/tslint-extends-default-severity.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"extends": [ "./tslint-default-severity-unspecified.json", "./tslint-default-severity-error.json" ],
"defaultSeverity": "warning",
"rules": {
"default-severity-warning": { "severity": "default" }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add another config that has no defaultSeverity, that extends this one.
The test then checks that rules specified in the newly added extending config get a severity of "error" while all 3 rules coming from this config are warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please suggest the name for this file. I could come up only with tslint-extends-default-severity-only-in-extended.json.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just take the name you suggested. I cannot come up with a better one.

20 changes: 18 additions & 2 deletions test/configurationTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,22 @@ describe("Configuration", () => {
const actualConfig = extendConfigurationFile(baseConfig, extendingConfig);
assertConfigEquals(actualConfig, expectedConfig);
});

it("overrides defaultSeverity of base configs", () => {
const config = loadConfigurationFromPath("./test/config/tslint-extends-default-severity.json");
assert.equal<RuleSeverity | undefined>(
"warning",
config.rules.get("default-severity-unspecified")!.ruleSeverity,
"should apply defaultSeverity to base config with no defaultSeverity");
assert.equal<RuleSeverity | undefined>(
"warning",
config.rules.get("default-severity-error")!.ruleSeverity,
"should override defaultSeverity defined in base config");
assert.equal<RuleSeverity | undefined>(
"warning",
config.rules.get("default-severity-warning")!.ruleSeverity,
"should apply defaultSeverity to extending config");
});
});

describe("findConfigurationPath", () => {
Expand Down Expand Up @@ -283,11 +299,11 @@ describe("Configuration", () => {
assert.equal<RuleSeverity | undefined>(
"error",
config.rules.get("no-fail")!.ruleSeverity,
"did not pick up 'no-fail' in base config");
"should pick up 'no-fail' in base config");
assert.equal<RuleSeverity | undefined>(
"off",
config.rules.get("always-fail")!.ruleSeverity,
"did not set 'always-fail' in top config");
"should set 'always-fail' in top config");
assert.equal<RuleSeverity | undefined>("error", config.jsRules.get("no-fail")!.ruleSeverity);
assert.equal<RuleSeverity | undefined>("off", config.jsRules.get("always-fail")!.ruleSeverity);
});
Expand Down