fix(@schematics/angular): revert breaking angular-whitespace lint rule

[cyrilletuzi]

While updating to Codelyzer 4.0, #257 also added a new angular-whitespace rule.

This must be reverted because :

• it is a breaking change : existing project are now failing on lint ;
• it is opinionated (it is not in official Angular style guide) and so should not be forced ;
• it is the contrary of what is done in the official Angular doc and in the style guide itself.

@hansl @filipesilva @Brocco @mgechev

[cyrilletuzi]

Related : #292 should be reverted.

Btw, note that #292 forgot to update the inline version of AppComponent. Similar problems (differences between external and inline AppComponent template) have happen many times before, tests should really include both cases.

[wKoza]

If it's more simple, you can take this PR #288.

[mgechev]

I agree it could be considered opinionated. It's described in the codelyzer's docs so people can use it as a reference if they decide to reintroduce the rule.

[clydin]

The second two points are definitely valid arguments for the rule’s removal.
However, its inclusion should not be considered a breaking change as only new projects would use the new rule. Existing projects would need to manually add the rule to be affected.

[cyrilletuzi]

Existing projects would need to manually add the rule to be affected.

People (like me) who want to stay synced with the CLI updates can do that. Yes, it is very painful (as they are no changelog), but it's one good reason to not make this process even more painful.

You can also create a new project and want to reuse some code from another project and then it will break lint.

Or just very simply : you're used to no whitespace, you start a new project and now you have to change your coding style.

So yes, it's not a breaking change in the sense of breaking existing projects, but it's breaking in the sense it changes an existing behavior in such way it can produce lint failure (proof is : part of the schematics had to be updated accordingly).

[mgechev]

The rule has autofix so making sure your project is compatible with the new angular-whitespace will be just a matter of running the linter with fix flag.

[cyrilletuzi]

Another use case : I'm teaching Angular courses, I'm vigilant to follow the Angular style guide. So now I have to change all my course materials, or students will copy the code as in the material and the lint will error them (and as #269 is still not fixed, it will be red errors).

[mgechev]

@cyrilletuzi as I mentioned here, in the style guide we don't enforce syntactical conventions. There's no recommendation for the interpolation syntax. The snippets are not surrounding interpolated expressions with whitespace because that's the style their author is using.

Again, I don't mind dropping the rule if the majority thinks it's too opinionated. :-)

[cyrilletuzi]

in the style guide we don't enforce syntactical conventions

Then the linter shouldn't too.

[mgechev]

@cyrilletuzi the Angular style guide and a coding style guide are independent concepts. The linter cannot be dropped because it enforces common coding style in teams. On the other hand, the Angular style guide points out best practices and warns about common gotchas when using the Angular's API.

In this particular case both are verified by a single tool - tslint. Keep in mind that there are two not intersecting sets of tslint rules with different purposes. There are style focused rules and functionality/maintainability focused ones which respectively map to the coding style and the Angular style guide.

Of course, the coding style as well as the style guide rules are also a matter of personal taste. That is why the tool is configurable so everyone can adjust it accordingly.

[wKoza]

@cyrilletuzi , If I follow your reasoning, we can't improve the tslint ? angular-whitespace is a pure lint rule (with an auto fix). Like the other rules, it can turn off. But Why ?

[cyrilletuzi]

The discussion has to take #269 discussion into account, otherwise we won't understand each others. On this other discussion, everyone argue that lint problems should be errors.

Changing important linting rules in a minor version would be quite OK if errors were displayed as warnings in the editor (even if it would not be my opinion). Currently they are displayed as errors and so it's not OK, it feels like a breaking change, as already explained.

Leading to this real situation : on my next courses, students will create a new project with the CLI, and then try to use content from the course materials (which for now use no whitespace), and the editor will report them errors. That's not acceptable.

Another more common situation : people will create a new project with the CLI, copy/paste code from the official Angular docs, and the editor will report them errors. That's not acceptable too.

Even if there is an autofix : the autofix is not enabled by default (and should not), and the idea of the CLI is to be ready to use.

Even if it's not an official style rule of Angular : it's just not reasonable that the official doc is written with one convention and the CLI enforces the other. I don't even understand why there is a discussion about this.

[mgechev]

@cyrilletuzi that's a good reminder. The good news is that angular.io is using the CLI, which means that this rule will be applied there as well so the style guide will be properly aligned to the update.

[wKoza]

Concerning #269, for me, a lint error must be an error for a simple reason : If it's not an error, and that it doesn't break the CI, It's never fixed by developers.
Except for one: I have add the rule "deprecation" #301 with level "warn" for an unique reason: resolution time can be long and complex

[hansl]

Okay, reading all this discussion, I think it's fair to say that:

1. this is an opinionated decision. This is not the first time we make an opinionated decision, but might have been going too far. We can stay compatible with the rule without enforcing it anyway.
2. this is NOT a breaking change. I do not consider your usecase "normal", as you shouldn't recreate a new application every time you upgrade the CLI. The CLI was made with backward compatibility and we strictly enforce semantic versioning. So you should only create an application once.
3. errors are better than warnings for lint. If you want warnings, it's not hard to change the tslint.json file in your own project, but I'd say fix(@schematics/angular): set tslint to emit warnings #269 will most likely be closed.

I'm going to approve this PR following those 2 points. But I want you to keep in mind point number 2 above; there's no upgrade path outside of updating your package.json file (for now).

We are planning a command to update your project if there are changes that require attention.