Proposal: Conditional lint configuration

[calebegg]

There are some situations where we'd like to conditionally enforce a lint check based on whether the code being linted is test code or not. For instance, we'd like to ban 'any' from production code, since it's unsafe. But in tests, 'any' can be useful to supply fake or mock implementations of services, or to cast through it so you can call private methods. Or we might want to ban a different set of methods in tests vs in production code, for example.

What I propose is a way to conditionally enable lint checks based on a regex match on the filename. This could also be useful for turning off lint checks for certain directories (containing "legacy" code, for instance), or for .tsx or .d.ts files, etc.

Here are some syntax strawman ideas; I'm open to suggestions too.

Sections object

{
  "rules": {
    "no-any": {"other": true},
    "no-console":
        {
          "test": [true, "warn", "error"], 
          "other": [true, "log", "warn", "error"]
        }
  },
  "sections": {
    "test": ".*\\.spec\\.ts$"
  }
}

"Other" would be things that don't match any of the regexes. This is pretty concise, but I think the readability of the rules section suffers a bit.

"Override" rules

{
  "rules": {
    "no-any": true,
    "no-console": [true, "log", "warn", "error"]
  },
  "override": {
    "match": ".*\\.spec\\.ts$",
    "rules": {
      "no-any": false,
      "no-console": [true, "warn", "error"]
    }
  }
}

(this should perhaps be an array, to allow for multiple overrides too).

[ajafff]

If you put your tests and production code in separate directories, you wouldn't need such a feature. If you do not provide a config file as CLI argument, TSLint uses the nearest tslint.json. Therefore you can have different settings for different folders. By using "extends" you can use the same base config and override only specific rules.

I get that many Angular and React projects put tests in the same directory as the corresponding production code. Personally I think this is a bad practice. I've seen enough people (accidentally) importing symbols from tests in production code.

IMO the proposed feature adds a lot of complexity. The implementation may not be that hard, but it makes the config file difficult to read and understand.
If we are going to implement this, I'd prefer the second suggestion.

We'd also need to discuss how it works when extending configurations. In which order are the overrides applied?

[alexeagle]

True, you could just separate the code, but it hurts ergonomics. Either you have

src/
  tslint.json
  a/
    b/
      c/
test/
  tslint.json (extends ../src)
  a/
    b/
      c/

and c/foo.spec.ts has to import {symbol} from '../../../a/b/c/foo';

That path gets ugly in our monorepo where files are often very deep in the tree.

or you have

  a/
    src/
      tslint.json
    test/
      tslint.json (extends ../src)
    b/
      src/
        tslint.json
      test/
        tslint.json (extends ../src)
      c/
        src/
           tslint.json
        test/
          tslint.json (extends ../src)

which is too many tslint.json files

[calebegg]

On the topic of overrides: I'd say the overrides apply in the same order rules are applied. I don't know what that is, and I couldn't find documentation, but intuitively I'd think it'd be like this:

// a.json
{"rules": {"foo": [true, 1]}}

// b.json
{"rules": {"foo": [true, 2]}}

// c.json
{
  "extends": ["a.json", "b.json"],
  "rules": {"foo": [true, 3]}
}

would result in rule "foo" having the argument 3. If that rule wasn't in c.json, then "foo" should have value 2, because of the order of the "extends" array.

For overrides, I'd expect the same order to be applied. It makes sense to me to use the last override that matches the supplied regex, taking into account the order in which 'extends' rules are considered. A single file example:

{
  "rules": {"a": [true, 1]},
  "override": [
    {"match": "test|spec", "rules": {"a": [true, 2]}},
    {"match": "test", "rules": {"a": [true, 3]}}
  ]
}

would have value 2 for rule "foo" when the file path contains "spec", 3 if it contains "test", and 1 otherwise. (obviously, not a very useful set of overrides, since "test" might as well be omitted from the first, but you get the idea)

Does that make sense and answer your question?

[ajafff]

@calebegg There are some other edge cases to consider:

// a.json
{
  "rules": {"foo": [true, 1]},
  "override": {"match": "test|spec", "rules": {"foo": [true, 2]}}
}

// b.json
{
  "extends": "./a.json"
  "rules": {"foo": false}
}

What's the expected result?

• 2 because the order is a.json/rules -> b.json/rules -> a.json/override
• false because the order is a.json/rules -> a.json/override -> b.json/rules

Same here, what is the expected result:

// c.json
{
  "rules": {"foo": [true, 1]},
  "override": {"match": "test|spec", "rules": {"foo": false}}
}

// d.json
{
  "extends": "./a.json"
  "rules": {"foo": {"options": 2}},
  "override": {"match": "test|spec", "rules": {"foo": {"options": 3}}}
}

[ajafff]

Just a random thought: Why not just match the patterns in order and (partially) override the previous match:

{
  "rules": { // same as "*"
    "rule-one": true,
    "rule-two": true
  },
  "*.js?(x)": { // we could get rid of "jsRules" with this
    "rule-two": false
  },
  "*.{t,j}sx": {
    "jsx-rule": true
  },
  "*.spec.*" {
    "rule-one": false
  }
}

Some examples:

• foo.ts: rule-one, rule-two
• foo.tsx: rule-one, rule-two, jsx-rule
• foo.js: rule-one
• foo.jsx: rule-one, jsx-rule
• foo.spec.tsx: rule-two, jsx-rule
• foo.spec.js: none

I just don't know where to put this inside the config. The same level as "rules" makes it easier to write, but may confuse users, because it's on the same level as "extends", "rulesDirectory", ...

Another problem is extending configs. If we apply all overrides from the base config and then the overrides from the current config, it's rather difficult to understand.

It also makes the code more complex. Currently the configs are merged during parsing. With this proposal the final config is only known when we have a file name.

[minomikula]

Hello,

This feature will be great. I think Eslint is supporting this feature: https://eslint.org/docs/user-guide/configuring#configuration-based-on-glob-patterns. So, syntax and semantic could be the same, to avoid confusion.

[ajafff]

@minomikula Thanks a lot. I searched the ESLint docs before but didn't find this section.

Their approach makes sense. To summarize:

• "overrides" section in the config contains an array of overrides
• an override can specify multiple glob patterns ("files"). if one of them matches, the config applies
• you can exclude files by glob pattern: "excludedFiles"
• glob patterns are relative to the config file they are specified in
• glob patterns always need to match the whole path, not just the basename
• overrides are processed in order, overriding the previous one
• when extending a config file, the base config is completely evaluated before continuing to the extending config
  • base.json/rules
  • base.json/overrides
  • extending.json/rules
  • extending.json/overrides

There are some things to consider when porting this behavior to TSLint:

• we should definitely disable extends and linterOptions in overrides
• we probably also want to disallow rulesDirectory
• do we want to allow rules and jsRules in overrides or do we deprecate jsRules in favor of an **/*.js?(x) override?

Thoughts or comments @adidahiya @calebegg @alexeagle @minomikula?

[calebegg]

Oh, wow, yeah, that approach seems reasonable to me. I should have also looked for prior art.

[Patrik-Lundqvist]

This would be an awesome addition! The eslint approach works great when conditionally disabling some rules in test files.

[alexeagle]

@calebegg do you still plan to push this forward when you find time?

[calebegg]

Yes, I'd definitely like to work on this. @ajafff Are you generally happy with starting with the ESLint approach and iterating in PRs? Or do you think there are design questions we should talk about first?

do we want to allow rules and jsRules in overrides or do we deprecate jsRules in favor of an **/*.js?(x) override?

It makes sense to me to deprecate, but I don't feel very strongly either way.

[ajafff]

@calebegg I'm fine with the ESLint approach. I implemented a very similar concept in my POC linter runtime https://github.com/fimbullinter/wotan/tree/master/packages/wotan#overrides

Integration in the current TSLint API could be tough, because currently it merges all configurations while parsing. This needs to be deferred until the file's name is known and needs to be done for every file.

This is certainly a breaking API change.

[giladgray]

@calebegg @mitchlloyd @ajafff @alexeagle i see little benefit in this feature over simply nesting tslint.json files to override outer configs. that feature already works today and doesn't require any complex API breaks. would you be offended if we rejected this request?

[DanielSchaffer]

Nesting does not solve the issues discussed here. Many people do want their tests in the same directory structure as the code it tests, and there is simply no good way to do that with tslint right now.

[mgol]

There are loads of projects putting spec files in the same directory as sources. This has become a best practice in many environments, Angular CLI generates a project structure like that etc. I don’t think it’s realistic nesting tslint config files is going to work for all these people.

[hoovercj]

I agree that my project structure should not be dictated by a linting tool. Instead the linting tool should support common existing patterns. Colocating tests and source files is a common pattern.

[mitchlloyd]

@giladgray The requested change supports a scenario where users put their tests and production files next to each other like this:

some-dir/
  my-component.ts
  my-component.test.ts

How would "nesting tslint.json files" address this use case when we want different rules for my-component.ts and my-component.test.ts?

[insidewhy]

@giladgray angular guidelines recommend you put your tests alongside your files. So 99% of angular projects do this. Due to tslint being backwards every angular project I worked on had issues. Your last comment is confounding and myopic. If you can't/don't want to accept this incredibly useful suggestion then I think you are far too conservative to be the one making decisions for us on such an important project. This issue has been making our angular project work difficult for years.

[DanielSchaffer]

@ohjames Let’s stay on topic and avoid veering toward personal attacks, okay? Suggesting that someone isn’t fit to maintain their own project is not the way to influence OSS, and certainly isn’t going to help what we’re all arguing for here.

[johnwiseheart]

It makes sense to want overrides for files that don't fit into a nested structure as mentioned above. It seems to me like @giladgray's comment was intended to determine if there was still significant community interest in this change, given that there had been no activity on this issue for more than 6 months. This is specifically evidenced by his comment on the open PR (#3708):

if you do want to pursue this, please update this branch so we can review it, or close it if no longer relevant. we will close this if we do not hear from you in two weeks.

@ohjames I'd suggest you reconsider your comment - as @DanielSchaffer mentioned, personal attacks are not going to help you here, or anywhere in open source software. Everyone is trying their best to create good, reliable and feature-rich software, and attacking people in no way contributes to that.

[k-g-a]

Is there any update on this topic?
Idea proposed by @ajafff in his summarizing comment seems reasonable and satisfies all variety of needs. Using same semantics as eslint is a good way forward.

[millimoose]

As I've commented on the related #1063, my use case would be that using tslint-microsoft-contrib currently bombs on Vue single-file components for some rules. This might either be a Vue issue, or a TSLint issue, and those teams might or might not get around to fixing the specific problem - disabling the offending rules until that happens seems like a simple workaround that's not too cumbersome. Compared to either having to add the same disable directive to every single .vue file, and update it in all those places as the rule support changes. It also doesn't make sense to split up a project into .vue and .ts files, it would be a very awkward structure.

[RoystonS]

This issue is currently flagged as 'Needs proposal', but I think we already a proposal in #3447 (comment)?

[JoshuaKGoldberg]

@RoystonS @ajafff's proposal mentioned a few loose ends that should be addressed:

There are some things to consider when porting this behavior to TSLint:

• we should definitely disable extends and linterOptions in overrides
• we probably also want to disallow rulesDirectory
• do we want to allow rules and jsRules in overrides or do we deprecate jsRules in favor of an **/*.js?(x) override?

There should also be consensus from the folks in the palantirtech org on this before moving forward, since it's a pretty substantial feature.

[alfaproject]

Just stumbled upon this myself, and just want to voice my interest for this kind of feature.

We ended up relaxing some linting rules that affect production code and let's see how it goes. ):

[VincentLanglet]

I'd love to see this feature

[thejuan]

subscribing - Also need this.

[Mboulianne]

I need this so badly :(

[vidal7]

Same here! We are using typescript with Vuejs and our unit test files are not together in a tests directory but everywhere along component source code.

[JimBeem]

Would love to see this feature, we're currently using a slightly wonky setup that results in vscode highlighting different linting errors than what our command line lint highlights to get around this issue.

[redevill]

I Propose to apply the concept git repository uses for files in the .gitIgnore... to the tsLint rules. Usage of your current hierarchal position as context to the linter, along with an override file, when linting a particular subdirectory. Then you can include or exclude, setup any config at any directory level you want.

e.g.
A tslint config is placed in the root, and at any particular level in your source code structure, you would like a different behavior, simply override it with an extension tslint file in the root of that subdirectory.

[VincentLanglet]

@redevill It does help people writing test in a test folder. But not one writing test as something.test.ts in the same folder of the component/service.

[redevill]

Agreed - but with minor change: (Which could be automated with template generator (CLI Style)
MyComponentDir
---MyComponentTestsDir
------tslint.test.json
------something.test.ts
---mycomponent.component.css
---mycomponent.component.html
---mycomponent.component.ts

The idea could still work.

[matthewcorven]

Huge fan of this proposal. YES PLEASE! My preference would be the Overrides concept.

[RoystonS]

Btw, given the deprecation announcement of tslint, I can't really imagine that such a large change will be made at this stage, and my team's moving over to eslint, which already has per-rule + per-file override facilities.

[DanielSchaffer]

The aforementioned announcement: https://medium.com/palantir/tslint-in-2019-1a144c2317a9

[adidahiya]

@RoystonS is basically right, such a large feature will not be added to tslint at this point. see #4534

[abhishekgoenka]

We use an Angular project. We need the overrides concept. Otherwise, it is pain to manage