Overriding rule severity of recommended config preset is cumbersome and verbose

[felschr]

Bug Report

• TSLint version: 5.1.0
• TypeScript version: 2.2.2
• Running TSLint via: CLI & VSCode

tslint.json configuration:

{
    "defaultSeverity": "warning",
    "extends": [
        "tslint:recommended"
    ],
    "jsRules": {},
    "rules": {},
    "rulesDirectory": []
}

Actual behavior

All issues are reported as "ERROR".
Examples of those are: "Missing trailing comma", "Missing trailing comma", "Missing trailing comma"

Expected behavior

Depending on the issue they should have different types. I expected most of them to be warnings.

[adidahiya]

This is intended behavior. We want TSLint failures to fail our builds, so we treat them as errors.

You can override any of the severities without changing the rule configuration inherited from tslint:recommended. And then you can even make your own reusable configuration preset from that.

[adidahiya]

Example to illustrate my above comment:

{
  "extends": "tslint:recommended",
  "rules": {
    "trailing-comma": { "severity": "warning" }
  }
}

[Bnaya]

On that way i need to have a reference to every rule i'm extending on my inheriting file

[MrAndersen1]

I have the same "issue". I want errors to be reserved for actual errors, so I find myself adding all tslint-rules to my file that reports things as errors, because to my knowledge there is no override for this. But if that is the way it is supposed to be then I'll just keep adding :) still love tslint!

[adidahiya]

@Bnaya @MrAndersen1 ah, sorry about that, I missed this nuance. I think we should allow defaultSeverity to override all the configs you are extending. See my comment here #345 (comment)

I'll update this thread with a more descriptive title

[nchen63]

I was planning on addressing this (if there was an outcry) with either another property or changing the defaultSeverity options. The reason it is implemented this way (affects only the current file) was to allow for these future scenarios:

• Scenario A: (current behavior) The extended rule set has a curated set of severities that the user wants to retain, and uses defaultSeverity to affect only the current file.
• Scenario B: The extended rules have a mix of default/warning/error severities and the user wants defaultSeverity to affect the inherited rules with severity=default AND defaultSeverity is set to default in the extended config.
• Scenario C: The user wants to wipe out all inherited severities.

This hypothetical 2nd property, say defaultSeverityModifier could address all of these with settings current, all, and override-inherited.

I could possibly be convinced that we only care about Scenario B and forget about the 2nd property

For completeness, I think the following scenarios don't need to be addressed and add unnecessary complexity:

• Scenario D: defaultSeverity is inheritable. This would mean that if an extended config specifies defaultSeverity and the current file does not, then it applies the severities to the current file too.
• Scenario E: defaultSeverity in the current config retroactively overwrites the extended config's defaultSeverity even if it was set to error or warning. That is, the extended config acts as though it was configured to use the current config defaultSeverity

[postspectacular]

+1 for Scenario B - I understand the reasoning for A, though it's not as flexible (and further complicated by the choice of using "error" as global default severity). To have "warning" as global default, my temporary workaround is to copy all tslint:recommended rules into my own config...

[kylecordes]

This is indeed a surprisingly big inconvenience compared to older behavior. While working in an IDE it is extremely helpful to have clear differentiation between language errors (and the occasional most severe kind of lint error) versus lint recommendations. Many editors and IDEs can hopefully present these differently while developing. For example with Visual Studio code, red versus green marks.

Failing a build because of lint is indeed a valuable behavior, but it shouldn't be necessary to lose the differentiation of warning in error throughout the whole development toolchain just to achieve this. How about if the lint invocation had an equivalent to old-style compilers -W option meaning "treat warnings as errors"? That way the configuration could nicely differentiate all the way through, and only at the point of wanting to fail a build, would the choice be made to treat warnings as errors in (only) that context?

[borekb]

Maybe the original post wanted to include an example with a warning instead of an error?

{
    "defaultSeverity": "warning",
    "extends": [
        "tslint:recommended"
    ],
    "jsRules": {},
    "rules": {},
    "rulesDirectory": []
}

This is something I've tried to suppress the hard errors in VSCode after updating to TSLint 5.x. (Also mentioned here: #345 (comment).)

[kylecordes]

I'm curious whether any tslint core folks want to weigh in on whether a -W option or similar to treat warnings as errors, is likely to be added? I'm trying to decide whether to create such a thing in a hacky manner (by parsing the output and looking for errors versus warnings), but won't bother if an official solution is likely.

(This item is marked "accepting PRs", but I think that notion applies to the feature is described in the title, the ability to adjust rule by rule. That is a great idea! ...but doesn't release help at all with the problem that -W would solve.)

[felschr]

So, I had some time now and created a pull request for this: #3240
The tslint:recommendation configuration has no defaultSeverity or severities per rule set.

So in my view when I extend my configuration with tslint:recommended the defaultSeverity that I set in my configuration should also apply to the configurations that reference in extends.

So, basically this would be @nchen63's Scenario B mentioned here: #2569 (comment)

I would like some feedback if you think if this is a step in the right direction or if something like @kylecordes mentioned (forcing the severity "warning" on all rules with a flag) would be better / should also be added.

[kylecordes]

@felschr To clarify, the feature I request and recommend, is what many compilers and lint tools have offered for many years:

• Have well crafted default, and perhaps some user settings, as to what is treated as warning vs error.
• Make them mostly warnings - that is implied by it being a "lint" tool, few developer if given a choice, will want lint issues showing up the same as compilation errors, in their IDE.
• Don't 'contaminate' these warning-vs-error issues, with the separate matter of whether it makes sense in a build pipeline to fail-on-warning.
• Offer a command line flag to "treat warning as errors for the purpose of failing lint".

With such a design, I see no reason for a defaultSeverity setting to exist. The reasonable defaultSeverity is warning, once you have the ability to do the "make lint problems fail the build" as describe above.

[ildoc]

treating rules violation as errors should be optional and imho "green" warnings should be the default behaviour

this is pretty annoying:

[sarink]

👍 to this comment

@adidahiya This suggestion doesn't work - it seems to just disable the rule completely?

I've always thought of an error as meaning "this is going to explode", and a warning as meaning "this works, but you should do it better". I actually can't quite comprehend how this opinion is so unpopular (or why the defaults for both eslint and tslint are the way they are).

Also if you want your build to fail (for, eg: whitespace), why not just make it fail on warnings?

Is there anyone who has a tslint.json which turns many of these """errors""" into warnings?

[adidahiya]

@sarink if my suggestion doesn't work to change the severity of a rule, that sounds like a bug. Please open a new issue if you have an isolated repro.

For others in this thread -- if most lint failures should be considered "warnings" (I tend to agree), then which lint rules do you think ought to be considered errors by default? Yes, this is a "lint" tool; nothing that TSLint reports will make your program explode and every rule can be disabled inline in source code.

@kylecordes I appreciate your well thought out proposal, but I think crafting such a default configuration will be quite subjective and involve a lot of bikeshedding. I think #3449 is sufficient to solve most of the UX issues here. We can make tslint:recommended and tslint:latest use defaultSeverity: "warning" in the next major version of TSLint.

[MrAndersen1]

As I only use TSLint through VSCode all my issues with TSLint classifications would go away if TSLint issues didn't share a view with VSCode's "Problems" where it puts all the "actual" errors.

Actually one more issue, which probably is a plugin issue, is that green squigglymarks takes precedence over VSCode's typescript-red-squiggly-errors so you think there's a lint-issue on that line when in fact there is a typescript error as well.

In short: If TSLint could report issues, in VSCode, in a way that it doesn't interfere with the errors VSCode/typescript reports then I would be happy. I don't have a suggestion for how to do this however.

Regardless, keep up the good work :)

[sarink]

@adidahiya #3474 - are you sure tslint was written to work this way? "Extend" seems to be a lie, as any rule configuration completely overwrites its parent rule, it does not merge or extend it.

[adidahiya]

@sarink we don't deep merge rule configs, no. But your observed behavior of "it seems to just disable the rule completely" sounds like a bug. This syntax is not expected to disable the trailing-comma rule:

{
  "extends": "tslint:recommended",
  "rules": {
    "trailing-comma": { "severity": "warning" }
  }
}

[sarink]

@adidahiya I misspoke when I said "disable completely' - what I should've said is that if the parent (eg, "tslint-config-airbnb") had applied special options to the rule (eg, [{ "multiline": "always", "singleline": "never" }] - there seems to be no way to keep these default (parent) options as they were, and only modify the "severity".

It may also be worth noting that eslint does deep-merge rules, so people migrating from that may find the word "extends" a little misleading.

Personally, I think a big step in the right direction for this issue would be to tackle #3474, as that would at least allow a quick way to change the severity of any rule, and modify nothing else, by simply adding "ruleName": { "severity": "warning" }

[ajafff]

@sarink As @adidahiya said, it is possible to only change the severity while leaving the options unchanged. If that doesn't work, it should be reported in a separate issue.

[sarink]

@ajafff Is severity not considered part of the "rule config"? I assumed when he said "we do not deep-merge rule configs" that it meant that applying a severity would result in overriding all other configs, including options (that's also been my experience in using it).

[ajafff]

severity and options are handled separately. You can override severity without changing options. If you only set options, severity is reset to "default".

[sarink]

Hmm, ok, I misunderstood then. I will try to isolate this and open a new issue, because it doesn't seem to be working for me.

[adidahiya]

Fixed via #3449 and #3530

[JoshuaKGoldberg]

🤖 Beep boop! 👉 TSLint is deprecated 👈 and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋