feat(@angular/cli): tslint5 severity option for each lint rule

[cyrilletuzi]

cli 1.1.0 upgraded to tslint 5, but tslint 5 is now raising errors by default, instead of warnings previously, and it's very confusing (lint errors can't be on the same level as syntax errors).

[clydin]

Before 5.0, there were no errors or warnings; only failures. Semantically, failure and error seem more equivalent than failure and warning.

I think the larger issue may be how tools and plugins that integrated with TSLint handle "lint" errors and warnings. For example, an IDE integration could treat lint errors as project warning messages and lint warnings as project info messages.

[cyrilletuzi]

I don't know about the larger issues, but as an Angular trainer I know what will happen to beginners with the 1.1.0 new configuration : they'll stay stuck on unimportant lint errors, because when they see red they always think "error" (even if I told them and repeat 10 times it's not always real errors).

Warnings was the previous behavior in CLI 1.0, and as this change hasn't been documented in the changelog, I suppose this was a side effect of tslint upgrade, and not a wanted change.

Can someone relaunch the checks ? They failed just because I forgot a ":" in the PR title.

[clydin]

I'm not actually opposed to the change. I just thought the issue could benefit from some additional background as the situation does have some depth to it. The VSCode TSLint plugin, for instance, changes it's behavior if 5.0 is used and issues errors or warnings based on the rule severity instead of previously always issuing warnings. Not that this is wrong behavior. However, there were multiple cascading changes that resulted in the behavior you are trying to prevent. (Also note that the plugin has a setting to revert to the previous behavior: alwaysShowRuleFailuresAsWarnings).

In regards to stylistic rules, I think they should definitely default to warnings. However, there are other rules that are far more akin to errors than warnings. This code for instance:

if (a => 2) {
  return true;
}

That is completely valid code and will compile and run but the conditional is always true. The strict-boolean-expressions rule will report the issue and would seem more appropriate as an error than anything else.

Regardless (and personally), I think it is actually a good thing that new developers spend some time looking at lint failures and actually understanding why they happen; and then learn when they actually can be disabled/ignored. Learning how to write good, quality code can drastically reduce the total lifecycle cost of a project. But having mentored numerous junior engineers, I do understand and appreciate your point.

[clydin]

For the CI failure, the actual commit message needs to be updated. The checks will re-run on the push.

[cyrilletuzi]

Yes, severity could be adjusted by rule. But if we go in this direction, we should be very careful, because in Angular it gets tricky quickly : for example, in one of my project, I have an error which is a false positive (a template reference that tslint doesn't see, but which is there and works very well at runtime). codelyzer had a lot of similar problems in the beginning. Now it's better, but new false positives happen regularly when Angular introduces new syntaxes in new releases.

[filipesilva]

@cyrilletuzi I see where you're coming from and I can't say I disagree. But it is ultimately a TSLint usability concern... and it would be much better to have it addressed by TSLint than hidden in new CLI projects imho.

@Brocco @hansl @sumitarora WDYT?

Regarding the CI failure, you'll have to re-push as @clydin mentioned.

[cyrilletuzi]

As a more general discussion : I was very impressed by CLI 1.0.0, because the generated project was clean and I could start coding the app without any config step. That's the very purpose of the CLI.

Since 1.0.0, many small but important changes were made (some in patch releases), sometimes with no prior discussion. And now with 1.1.0, after creating a project, I have several config steps to do to come back to a clean project. A few examples :

• put tslint back to warnings
• delete baseUrl in tsconfig to keep relative paths
• remove @angular/language-service package as it breaks VS Code Language Service plugin
• etc.

(I've opened issues for those other problems.)

The CLI feels quite unstable with such changes at every minor or even patch release.

[cyrilletuzi]

This time I don't know why the build failed. I will be unavailable for the next two weeks, so feel free to edit this PR.

[cyrilletuzi]

Can someone tell me why the appveyor build fails, so we can move on this ?

[cyrilletuzi]

I updated this PR to manage severity for each lint rule.

Decisions were made following this thinking :

• Errors for rules that could lead to real errors or performance issues.
• Warnings on pure style errors or few rules which can have valid exceptions.

It may be a good idea to add this somewhere in contributing guidelines for tslint rules which will be added in the future.

[filipesilva]

I'm sorry but I'm not going to allow this PR in. It adds a lot of extra bloat to the linting configuration for something that is meant to counteract decisions by the TSLint team and IDE implementations.

I understand the current situation affects your work as an Angular trainer, but I still think this is not the right place to make the change. It may temporarily help users making new projects with the CLI, but does nothing for everyone else.

If you feel strongly that the current lint IDE display is harmful to users, then the right place to bring it up is the issue trackers for the TSLint team and IDE implementations.

Regarding the other problems you highlighted, they will be addressed in the individual issues (#5875 and #6536).

I don't agree that the CLI feels unstable at all personally. As a maintainer, post 1.0 is by far the quietest period both in PRs and issues opened.

[cyrilletuzi]

Your decision, but I don't understand the reasoning.

tslint5 came with a new option for the user to decide the level of severity of each error. It's not tslint or IDE job to choose this for the user.

For me it feels as if you were saying that tslint would activate all the rules and it wasn't the user who could choose. Of course it is the user who have to choose, as the CLI does : some rules has been enabled and other disabled, as it depends on how each project uses TypeScript.

Same goes for severity level : depending on how you use TypeScript, it's more relevant for some rules to be errors and other to be just warnings.

[filipesilva]

I argue that the default severity (unless overwritten) is up to tslint, and how to display each severity level is up to the IDE. Changing the tslint config doesn't only change how it looks like in the IDE, but also how it's reported via ng lint/tslint.

For instance, setting "defaultSeverity": "warning", adding a debugger; statement, and running ng lint shows a warning and returns exit code 2. But running tslint -c tslint.json src/**/*.ts will show the warning and return exit code 0.

This difference is because we don't really cater for tslint warnings inside ng lint, but now that they exist we should.

But this would also mean that there's a bunch of lint rules that would not fail CI checks in Travis and such now, but would before. This isn't ideal IMHO. And I also don't think it's what you intended originally (but I might be wrong there).

Ideally, like @clydin mentioned in #6568 (comment), the better overall solution is that there is a way to not confuse compilation errors with linting errors in the IDE because that seems to be the real problem for users.

I'll bring this up further with the CLI core team though... this issue together with #6507 and our lack of warning handling mean ng lint needs to be reviewed.

[filipesilva]

Here is a topic on the VSCode TSLint plugin for the same topic: https://github.com/Microsoft/vscode-tslint/issues/199

[cyrilletuzi]

the better overall solution is that there is a way to not confuse compilation errors with linting errors in the IDE because that seems to be the real problem for users.

Yes, it was my intention with this PR.

But I didn't think of other tools like Travis, thanks for the explanation. Let me know if I can help after CLI team discussion.

[alexeagle]

internally at google we have different *.tslint.json files, which can extend from one another. The one for editors has "defaultSeverity": "warning" because I agree the red underline presentation is inappropriate, distracting, and teaches new TypeScript developers that the language is much stricter than it really is.
Then we have a couple more files for per-file and per-library linting which have differing "typeCheck": true/false

[cyrilletuzi]

Closing in favor of angular/devkit#269

Now there are schematics, so the problem between internal rules and rules for people using the CLI should be gone.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}