Override defaultSeverity defined in extended configs #3449

M0ns1gn0r · 2017-11-03T19:05:15Z

PR checklist

Addresses an existing issue: Overriding rule severity of recommended config preset is cumbersome and verbose #2569
Enhancement
- Includes tests
Documentation update

Overview of change:

Helping @felschr to finish the implementation he started in the PR #3240 (AFAIK it's not possible to add commits to somebody else's pull request). I took into account @ajafff's feedback and added the test case.

CHANGELOG.md entry:

[enhancement] Override defaultSeverity defined in extended configs (#2569)

palantirtech · 2017-11-03T19:05:16Z

Thanks for your interest in palantir/tslint, @M0ns1gn0r! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

ajafff · 2017-11-03T20:09:16Z

test/configurationTests.ts

+            assert.equal<RuleSeverity | undefined>(
+                "warning",
+                config.rules.get("default-severity-unspecified")!.ruleSeverity,
+                "did not apply defaultSeverity to base config with no defaultSeverity");


I was a bit confused because all other tests use the message the other way around. They describe what the test expects, not what caused it to fail. For example: assert.isNull(err, "process should exit without an error");

There's only one other test in this file that provides a message for assertions. Please change them too while you are here.

ajafff · 2017-11-03T20:31:57Z

test/config/tslint-extends-default-severity.json

+  "rules": {
+    "default-severity-warning": { "severity": "default" }
+  }
+}


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.

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

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

ajafff · 2017-11-03T21:25:17Z

Code looks good to me.

I just want to make sure this is what we want. Ping @adidahiya
To summarize the current behavior:

b.json extends a.json
- b has a defaultSeverity
- b overrides the defaultSeverity of a
d.json extends c.json
- only c has a defaultSeverity
- d does not inherit the defaultSeverity of c
- instead it defaults to "error" while everything from c has the defaultSeverity of c applied

adidahiya · 2017-11-07T20:13:47Z

src/configuration.ts

@@ -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"`.


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"

adidahiya · 2017-11-07T20:18:57Z

test/configurationTests.ts

+            assert.equal<RuleSeverity | undefined>(
+                "error",
+                config.rules.get("default-severity-only-in-extended")!.ruleSeverity,
+                "should not inherit defaultSeverity from base configs");


I don't think this is quite right. Let's say I inherit from a shared config file where defaultSeverity is set to "warning".

Then I change one of the rule configs to enable some specific rule option.

{ "extends": "config-where-failures-are-warnings", "rules": { "comment-format": { "options": ["check-space", "check-lowercase"] } } }

now, comment-format produces errors. this is mildly confusing UX. While I think people could generally live with it, I think defaultSeverity should ideally be inherited from the extendingConfig if it is not specified in the current config.

That's actually a bit more complicated to achive. We have a circular dependency here: we need to call parseConfigFile to know extended configs, but parseConfigFile would already need to know the defaultSeverity of the extended config file.
We need a bigger refactor of configuration.ts to make it work. I think this would result in a breaking change for API users.

Btw. which defaultSeverity is chosen when extending two or more config files? The last one?

which defaultSeverity is chosen when extending two or more config files? The last one?

yes, the last one in the extends list.

Do we agree that this change is better to be done in a separate PR?

@M0ns1gn0r I agree that this may be too much for a first-time contributor. Though the necessary changes may not be too big.
We should definitely add the missing bits shortly after landing this PR and before cutting a release.

@adidahiya we could make it work as you described by moving the extending of the config file from loadConfigurationFromPath into parseConfigFile. That way we could know the defaultSeverity of the parent config before it is used in parseRuleOptions.
IMO the best way to implement this backward compatible is by adding an optional parameter to parseConfigFile. That parameter is a function to load a config file by path. If this parameter is passed, parseConfigFile resolves all extended configs and reduces them. Otherwise the config is returned without touching extends.

@ajafff your proposal sounds reasonable to me. We might want to do this work on a feature branch so that master is not left in an in-between state (the Github UI supports re-targeting PR branch destinations).

I had to refactor quite a bit of the code from this PR, but here it is: #3530

ajafff · 2017-11-08T18:59:17Z

src/configuration.ts

@@ -25,7 +25,8 @@ 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 config file as well as rules
+     * in any inherited config files which have their severity set to "default".
     * Not inherited.
     */
    defaultSeverity?: RuleSeverity;


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.

It is used on lines 70 and 78:

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

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.

ajafff · 2017-11-08T19:07:56Z

test/configurationTests.ts

+            assert.equal<RuleSeverity | undefined>(
+                "error",
+                config.rules.get("default-severity-only-in-extended")!.ruleSeverity,
+                "should not inherit defaultSeverity from base configs");


@M0ns1gn0r I agree that this may be too much for a first-time contributor. Though the necessary changes may not be too big.
We should definitely add the missing bits shortly after landing this PR and before cutting a release.

@adidahiya we could make it work as you described by moving the extending of the config file from loadConfigurationFromPath into parseConfigFile. That way we could know the defaultSeverity of the parent config before it is used in parseRuleOptions.
IMO the best way to implement this backward compatible is by adding an optional parameter to parseConfigFile. That parameter is a function to load a config file by path. If this parameter is passed, parseConfigFile resolves all extended configs and reduces them. Otherwise the config is returned without touching extends.

M0ns1gn0r · 2017-11-10T18:11:56Z

Is there anything left that prevents merging of this PR?

ajafff · 2017-11-10T21:48:13Z

@M0ns1gn0r currently there's nothing you can do. I'd like @adidahiya to sign off the changes I proposed so I know how to proceed after merging this.

adidahiya

sorry for the delay here!

adidahiya · 2017-11-28T22:32:32Z

docs/usage/configuration/index.md

@@ -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.


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.

adidahiya · 2017-11-28T22:35:02Z

test/configurationTests.ts

+            assert.equal<RuleSeverity | undefined>(
+                "error",
+                config.rules.get("default-severity-only-in-extended")!.ruleSeverity,
+                "should not inherit defaultSeverity from base configs");


@ajafff your proposal sounds reasonable to me. We might want to do this work on a feature branch so that master is not left in an in-between state (the Github UI supports re-targeting PR branch destinations).

ajafff · 2017-11-29T19:23:37Z

@M0ns1gn0r to fix CI you need to merge the latest master into your PR

I'll open a PR based on this one that adds the missing functionality. We can then merge this PR and mine right after that.

Edit: follow-up PR is #3530

…ride-default-severity

Override defaultSeverity of extended configs (palantir#2569).

251640b

Fix "allow-boolean-or-undefined".

97dbcf8

ajafff reviewed Nov 3, 2017

View reviewed changes

Negate assertion messages.

7dd26fc

ajafff reviewed Nov 3, 2017

View reviewed changes

Add test case "no inheritance of defaultSeverity ".

f9a9b93

ajafff approved these changes Nov 3, 2017

View reviewed changes

adidahiya reviewed Nov 7, 2017

View reviewed changes

Update defaultSeverity docs.

75587bd

ajafff reviewed Nov 8, 2017

View reviewed changes

ajafff mentioned this pull request Nov 9, 2017

[breaking] remove defaultSeverity from IConfigurationFile #3469

Closed

Deprecate defaultSeverity in IConfigurationFile.

aa11a2e

adidahiya mentioned this pull request Nov 28, 2017

Overriding rule severity of recommended config preset is cumbersome and verbose #2569

Closed

adidahiya reviewed Nov 28, 2017

View reviewed changes

Update defaultSeverity description in index.md.

58497de

Merge branch 'master' of https://github.com/palantir/tslint into over…

04cb64e

…ride-default-severity

ajafff mentioned this pull request Nov 29, 2017

Inherit defaultSeverity and apply it to preceding base configs #3530

Merged

adidahiya modified the milestones: TSLint 5.x, TSLint v5.9 Jan 9, 2018

adidahiya merged commit ef28083 into palantir:master Jan 10, 2018

M0ns1gn0r deleted the override-default-severity branch January 10, 2018 19:26

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018

Override defaultSeverity defined in extended configs (palantir#3449)

f24ec8e

jkillian mentioned this pull request May 1, 2018

inherit defaultSeverity onto child configurations #3240

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override defaultSeverity defined in extended configs #3449

Override defaultSeverity defined in extended configs #3449

M0ns1gn0r commented Nov 3, 2017

palantirtech commented Nov 3, 2017

ajafff Nov 3, 2017

ajafff Nov 3, 2017

M0ns1gn0r Nov 3, 2017

ajafff Nov 3, 2017

ajafff commented Nov 3, 2017

adidahiya Nov 7, 2017

adidahiya Nov 7, 2017

ajafff Nov 7, 2017

adidahiya Nov 7, 2017

M0ns1gn0r Nov 8, 2017

ajafff Nov 8, 2017

adidahiya Nov 28, 2017

ajafff Nov 29, 2017

ajafff Nov 8, 2017

M0ns1gn0r Nov 9, 2017

ajafff Nov 9, 2017

ajafff Nov 8, 2017

M0ns1gn0r commented Nov 10, 2017

ajafff commented Nov 10, 2017

adidahiya left a comment

adidahiya Nov 28, 2017

adidahiya Nov 28, 2017

ajafff commented Nov 29, 2017 •

edited

Loading

Override defaultSeverity defined in extended configs #3449

Override defaultSeverity defined in extended configs #3449

Conversation

M0ns1gn0r commented Nov 3, 2017

PR checklist

Overview of change:

CHANGELOG.md entry:

palantirtech commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M0ns1gn0r commented Nov 10, 2017

ajafff commented Nov 10, 2017

adidahiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff commented Nov 29, 2017 • edited Loading

ajafff commented Nov 29, 2017 •

edited

Loading