feat(@angular/cli): extend tslnit config from recommended #6507

ValeryVS · 2017-05-30T15:17:17Z

no need to duplicate rules
new rules can be applied without updating, when new tslint is used

Fixes: #6179

ValeryVS · 2017-05-30T15:18:41Z

polifills.ts edited with jsdoc-format
https://palantir.github.io/tslint/rules/jsdoc-format/

Tests files edited with arrow-parens
https://palantir.github.io/tslint/rules/arrow-parens/

ValeryVS · 2017-05-30T15:20:52Z

Probably, some blueprints must be edited too.I will check this tomorrow. For now app generates without lint errors.

We can minimise blueprint changes and disable some rules.
And apply them in other PRs.

clydin · 2017-05-30T15:39:12Z

From the linked issue it looks like @cexbrayat was also working on this.
Also, the changes should ideally replicate the existing ruleset (i.e., no file changes should be necessary).

ValeryVS · 2017-05-30T22:00:37Z

@clydin

Also, the changes should ideally replicate the existing ruleset

Some rules just hasn't been used yet.
I don't think, that we should extend recommended config and disable a lot of rules from it.
But we can add support for these new rules in another PR.

cexbrayat · 2017-05-30T22:22:37Z

I was indeed working on something similar but I didn't find the time to test my PR properly to see if the ruleset was identical. I also thought like @clydin that ideally the PR should require no changes as a first step, and wanted to offer several alternatives for the new rules and the ones where the CLI differs from the recommended config. But your PR @ValeryVS is maybe a good first step to start a discussion with the maintainers, so thank you for submitting this 👍

ValeryVS · 2017-05-31T07:39:49Z

@clydin @cexbrayat
Summary, there are three options.

Make changes in templates code to support new rules.
Disable rules, that forces us to make changes in cli templates. Users, mostly, will be forced to make changes in their code if they want update tslint.json with new one. But, anyway, cli have not command to automatically update files.
Disable all rules, that wasn't used in old config.

ValeryVS · 2017-05-31T07:57:12Z

If we choose "Disable all rules, that wasn't used in old config", we should make another request with new rules support anyway.
If we just disable rules, users can think: "angular team doesn't recommend these rules".

@filipesilva Which way from comment above you prefer?

ValeryVS · 2017-05-31T08:03:21Z

@cexbrayat
I compare config with recommended
https://github.com/palantir/tslint/blob/master/src/configs/recommended.ts
and remove all matched.
If we want to go third way, then we should disable all rules, that weren't in config now.

filipesilva

Heya @ValeryVS, good work matching these all. There just one rule that I think we should modify:

        "no-consecutive-blank-lines": [
            true,
            2
        ],

It would get rid of some of the changes needed in polyfills, and overall it's quite common to leave two empty lines as separators (we do it a lot on the CLI).

Other than that, LGTM.

cexbrayat · 2017-06-01T12:58:56Z

I tried the new config quickly on an existing project, and it raises a lot of warnings due to arrow-parens which is false in tslint:recommended and array-type which forces to use simple array for simple types and generic arrays for type T. These two rules were not enforced previously.

I don't know if this is a good or bad thing, I just wanted to point it out. Other than that, it's really close to what I had in my WIP PR for this 👍

clydin · 2017-06-01T13:05:59Z

Personally, I really don't think arrow-parens should be a recommended rule.
I think something to also keep in mind is that the recommended rule set is tslint's opinion on what should be recommended and opinions tend to vary. As an example, TypeScript itself doesn't use the rule set: https://github.com/Microsoft/TypeScript/blob/master/tslint.json

ValeryVS · 2017-06-02T10:18:21Z

@filipesilva
Good point about no-consecutive-blank-lines. Changed.
There was three lines in polyfills.ts, so some changes are still there.

Also adjust no-trailing-whitespace rule. Using typescript repo as reference.

    "no-trailing-whitespace": [
      true,
      "ignore-template-strings"
    ],

@cexbrayat
I'm too must make some changes to support array-type, but it is useful

export interface Entity {
  users: {
    id: number;
    name: string;
    photo: string;
    birthDate: string;
  }[]; // <-- Pst... I'm array. Don't tell any one!
}

@clydin
arrow-parens is true in latest tslint:recommended
There are some argues about arrow-parens in eslint and tslint. ESLint has explanation:

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

This usage case is madness, but somtmies I swap lettres when tpying fast. So, it make sense.
http://eslint.org/docs/rules/arrow-parens

Anyway, the final word is for consumers. I, personally, add few more strict rules to this config for my projects.

I want to add semicolon rule, but, probably in second PR. I think in will require some changes in templates.

"semicolon": [true, "always", "ignore-bound-class-methods"]

We can discuss what else we could enable or disable in followup requests.

ValeryVS · 2017-06-02T12:44:35Z

tests/e2e/tests/lint/lint-with-type-check-fail.ts

@@ -30,7 +30,6 @@ function check(val: any, fxState: any) {
    fxState.num = val + "";
  }
 }
-


There was fail Exceeds the 2 allowed consecutive blank lines which not fixed with lint --fix command.
We can change no-consecutive-blank-lines to 3, if we want to avoid this change.

ValeryVS · 2017-06-02T12:44:43Z

tests/e2e/tests/lint/lint-with-type-check.ts

@@ -29,7 +29,6 @@ function check(val: any, fxState: any) {
    fxState.num = val + "";
  }
 }
-


There was fail Exceeds the 2 allowed consecutive blank lines which not fixed with lint --fix command.
We can change no-consecutive-blank-lines to 3, if we want to avoid this change.

clydin · 2017-06-02T13:34:50Z

The arrow-parens rule doesn't actually address the underlying issue with your example. The example conditional is just always true. The presence or lack of parenthesis wouldn't change that. The rule warning might cause the developer to re-examine the statement but the quick fix is to add parenthesis (which gives a false sense of correctness and bad information as to the actual problem). Also anyone using a lint with auto-fix setup (pre-commit hook, CI, etc.) would still have the underlying error of an always true conditional.
The arrow-parens rule is really a stylistic rule. To catch and handle the situation in the above example, strict-boolean-expressions is the appropriate rule to enable. It actually warns the user as to the actual problem. It is TS only and is best used in combination with strict null checks.

clydin · 2017-06-02T13:47:19Z

Also, the fact that there is debate here regarding various rules essentially proves my earlier point that even a recommended rule set is highly opinionated.

My overall view for the default rule set for the CLI is that it should encourage best practices especially in regards to the use of Angular but should not be overtly strict in general and especially in the realm of stylistic concerns. Ideally new users of the CLI shouldn't be turned off to the tool because their preferred code style immediately results in numerous errors.

With that said, I think a strict option for ng new could be interesting. This could enable a more opinionated tslint ruleset as well as some of TypeScript's addition settings (strictNullChecks, etc.).

filipesilva · 2017-07-03T11:58:10Z

Imho arrow-parens makes sense to have turned on in TS projects because of typings, since you can't do add them without the parenthesis (a: bool => {} does not work).

This, together with #6568 makes me think linting config in general needs to be reviewed... I'll try to bring this up and get it sorted one way or another.

Meanwhile, @ValeryVS can you rebase please? It's failing on stuff unrelated to your PR.

no need to duplicate rules new rules can be applied without updating, when new tslint is used Fixes: angular#6179

ValeryVS · 2017-07-26T11:21:06Z

@filipesilva
PR is rebased. Sorry for delay.

filipesilva · 2017-07-27T10:19:42Z

Heya @ValeryVS, wanted to give you an update. I escalated this up so that we come up with a better set of defaults instead of just trying to keep the current ones up to date.

The current set of defaults was based on something we used in the Angular.io examples a long time ago. Meanwhile a lot of changes have been introduced to tslint and codelyzer, including defaults.

We might end up making an angular default, from which projects would extend. I'm monitoring progress on this but have no estimate currently. Because of that I marked this as blocked.

rafaelss95 · 2017-10-04T22:31:50Z

@filipesilva What's the status on this PR?

hansl · 2018-02-22T20:20:18Z

Closing this; it changes blueprints which we don't support anymore. Please submit a PR on devkit if you want us to consider this.

angular-automatic-lock-bot · 2019-09-12T08:41:18Z

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.}

googlebot added the cla: yes label May 30, 2017

ValeryVS force-pushed the feat/tslint-extend-recommended-config branch from b370d8d to c770e77 Compare May 30, 2017 20:31

ValeryVS force-pushed the feat/tslint-extend-recommended-config branch from c770e77 to ab17c80 Compare May 31, 2017 08:04

filipesilva suggested changes Jun 1, 2017

View reviewed changes

filipesilva self-assigned this Jun 1, 2017

ValeryVS force-pushed the feat/tslint-extend-recommended-config branch from ab17c80 to d14c312 Compare June 2, 2017 10:10

ValeryVS force-pushed the feat/tslint-extend-recommended-config branch 3 times, most recently from 3d9e5cc to d698b1c Compare June 2, 2017 12:42

ValeryVS commented Jun 2, 2017

View reviewed changes

ValeryVS force-pushed the feat/tslint-extend-recommended-config branch from d698b1c to a0b477f Compare June 8, 2017 06:41

filipesilva mentioned this pull request Jul 3, 2017

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

Closed

nykula mentioned this pull request Jul 25, 2017

Externalise ts-lint rules wmonk/create-react-app-typescript#112

Closed

feat(@angular/cli): extend tslnit config from recommended

a4aeca7

no need to duplicate rules new rules can be applied without updating, when new tslint is used Fixes: angular#6179

ValeryVS force-pushed the feat/tslint-extend-recommended-config branch from a0b477f to a4aeca7 Compare July 26, 2017 09:58

filipesilva added the blocked label Jul 27, 2017

hansl removed the blocked label Jan 24, 2018

hansl unassigned filipesilva Feb 8, 2018

hansl closed this Feb 22, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(@angular/cli): extend tslnit config from recommended #6507

feat(@angular/cli): extend tslnit config from recommended #6507

ValeryVS commented May 30, 2017

ValeryVS commented May 30, 2017

ValeryVS commented May 30, 2017

clydin commented May 30, 2017

ValeryVS commented May 30, 2017 •

edited

Loading

cexbrayat commented May 30, 2017

ValeryVS commented May 31, 2017

ValeryVS commented May 31, 2017

ValeryVS commented May 31, 2017

filipesilva left a comment

cexbrayat commented Jun 1, 2017

clydin commented Jun 1, 2017 •

edited

Loading

ValeryVS commented Jun 2, 2017

ValeryVS Jun 2, 2017

ValeryVS Jun 2, 2017

clydin commented Jun 2, 2017 •

edited

Loading

clydin commented Jun 2, 2017

filipesilva commented Jul 3, 2017

ValeryVS commented Jul 26, 2017

filipesilva commented Jul 27, 2017

rafaelss95 commented Oct 4, 2017

hansl commented Feb 22, 2018

angular-automatic-lock-bot bot commented Sep 12, 2019

feat(@angular/cli): extend tslnit config from recommended #6507

feat(@angular/cli): extend tslnit config from recommended #6507

Conversation

ValeryVS commented May 30, 2017

ValeryVS commented May 30, 2017

ValeryVS commented May 30, 2017

clydin commented May 30, 2017

ValeryVS commented May 30, 2017 • edited Loading

cexbrayat commented May 30, 2017

ValeryVS commented May 31, 2017

ValeryVS commented May 31, 2017

ValeryVS commented May 31, 2017

filipesilva left a comment

Choose a reason for hiding this comment

cexbrayat commented Jun 1, 2017

clydin commented Jun 1, 2017 • edited Loading

ValeryVS commented Jun 2, 2017

ValeryVS Jun 2, 2017

Choose a reason for hiding this comment

ValeryVS Jun 2, 2017

Choose a reason for hiding this comment

clydin commented Jun 2, 2017 • edited Loading

clydin commented Jun 2, 2017

filipesilva commented Jul 3, 2017

ValeryVS commented Jul 26, 2017

filipesilva commented Jul 27, 2017

rafaelss95 commented Oct 4, 2017

hansl commented Feb 22, 2018

angular-automatic-lock-bot bot commented Sep 12, 2019

ValeryVS commented May 30, 2017 •

edited

Loading

clydin commented Jun 1, 2017 •

edited

Loading

clydin commented Jun 2, 2017 •

edited

Loading