fix(@schematics/angular): set tslint to emit warnings

[cyrilletuzi]

tslint 5 is now raising errors by default, instead of warnings previously, and it's very confusing (lint errors can't be displayed the same way as real syntax errors).

@hansl @Brocco @filipesilva

[filipesilva]

@cyrilletuzi won't this make running tslint always return successfully (instead of an error code)?

[clydin]

Based on the discussion in the referenced PR, I don't really think this should be default behavior especially since it is something that can be easily changed by the user, if desired.
To summarize: how lint errors are shown in an IDE is really a concern of the IDE. From the perspective of the linter, these really are errors.

[cyrilletuzi]

@filipesilva @clydin I just tested with ng lint and no, lint still fails, but errors are displayed like warnings instead of errors, as intended :

WARNING: /src/main.ts[6, 27]: " should be '
Lint errors found in the listed files.

[cyrilletuzi]

And even if it wasn't the case, I would disagree about the default behavior : if this is really easy to change, then default should be a nice experience for most users (which is the role of the CLI), who are currently troubled in their dev experience to see just lint errors as syntax errors (I'm teaching Angular courses, so I speak from experience), and advanced users should be the ones who have to change it (supposing it's necessary, but it doesn't seem so).

[filipesilva]

For reference, the PR that @clydin mentioned is angular/angular-cli#6568.

It surprises me that ng lint still errors out despite only having warnings. It might be some faulty logic on the CLI side, but an argument could also be made that this behaviour is better. Personally I think that ng lint should return the same return code as tslint.

On the topic of linting experience vs correctness, I'm sorry but I have to disagree. I understand that your experience overwhelmingly emphasizes the learning developer experience, but that the non-learning segment is also important. This default means no linting check will ever fail unless you change it manually. That includes CI checks, git pre-commit hooks, and just running ng lint (assuming ng lint mimics tslint exit code, which it doesn't right now).

You must get daily (if not hourly) confused questions about why all these linting problems show as errors, and for you this has been going on for months now. But please also consider how many issues would be opened because of ng lint/tslint usage that never errors out.

That is my argument: while the current option can be surprising, a lot of users will also find this change both surprising and incorrect. So I think this approach simply moves the bad user experience from one set of users to another, and then makes linting behaviour incorrect (insofar as linting is meant to fail when there are problems).

@alexeagle mentioned the multiple tslint approach (angular/angular-cli#6568 (comment)). We already do something similar with tsconfig files, so maybe that's another candidate solution. The bad part of it is that it adds more config file clutter. This is fine for big projects but bad user experience on small projects. Perhaps it's less bad than a window full of errors for quotes though.

And to be honest we already bend over backwards with exclusion lists to pretend we have multiple tslints in the .angular-cli.json linting config. So a per-app tslint plus a base one (like tsconfig) is not so far fetched. We might need a bunch of other per-app configs too (like karma and protractor config).

I think at the moment the current separation (or lack thereof) between editor tooling config and build tooling config is problematic for users, and that linting is just one facet of the problem.

[cyrilletuzi]

Well, the current CLI may be buggy, but wouldn't it be a good solution for both set of users ? With this PR, ng lint fails, but dev experience is OK, this seem perfect for everyone.

Another solution would be to include a default vscode (and other editors) setting file with "tslint.alwaysShowRuleFailuresAsWarnings": true. That's what I do for now, would it be ok ?

[hansl]

Just curious, why is this an issue now? Is is something new with 5.8? Looking through the changelog it doesn't seem to be something that changed in 5.8, and looking through our git blame we've been using 5 for months now.

[cyrilletuzi]

It's not an issue now. The initial PR is from 6 months ago. At that time I was told it was more complicated than I thought and that it would be discussed by the CLI team. Then no news, I just saw a bullet in one of the Weekly meeting notes saying it won't go any further if there was no caretaker for it.

[alexeagle]

Speaking of lint warnings vs. compiler errors, we now have another way to deal with this at Google, http://tsetse.info

[cyrilletuzi]

If I understand correctly new @hansl commit in CLI, the solution of this PR won't be possible anymore if we want to satisfy everyone.

So what about the second option ? Ie. generate a default settings file for main editors (like "tslint.alwaysShowRuleFailuresAsWarnings": true in VS Code) ?

We could add an option like editor=vscode on ng new to not bother people with different editors. (It could also allow other editor optimizations).

I can take care of the PR, just need a go (don't want to lose time if it's not OK, this one would be less trivial).

[hansl]

I'm going to close this as the intent behind linting is to show up as errors anyway, otherwise they're lost in the process. If you want to use warnings, doing this PR yourself in the project isn't too much work.