Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

A few rules are mostly redundant in newer TypeScripts with specific compiler options #1861

Closed
dead-claudia opened this issue Dec 13, 2016 · 12 comments

Comments

@dead-claudia
Copy link

Here's the relevant rules of varying redundancy unaddressed in the docs:

  • no-invalid-this--noImplicitThis (not a precise equivalent, but it makes all usages explicit)
  • no-switch-case-fallthrough--noFallthroughCasesInSwitch (exact equivalent)
  • no-unused-variable--noUnusedLocals and --noUnusedParameters (exact equivalent between the two)
  • switch-default is partially redundant with --noImplicitReturns, which requires that all code paths are reachable and return a value. (It only checks the end, but a final switch-case is rather common.)
@pe8ter
Copy link
Contributor

pe8ter commented Jan 18, 2017

If you deprecate options like no-unused-variable in favor of TypeScript compiler flags then you can't opt out with rule flags like // tslint:disable-next-line.

@nchen63 nchen63 added this to the TSLint 5.0 milestone Jan 18, 2017
@jsynowiec
Copy link

There is also an issue with --noUnusedLocals and --noUnusedParameters triggering when function signature has several parameters but you only use the last one, the first few are unneeded (but still must be declared).

For e.g.

function foo(a: any, b: T): T { return b; }

See microsoft/TypeScript#9458 for reference.

The approved fix is to prefix uninteresting parameters with an underscore, see microsoft/TypeScript#9464 which breaks the variable-name TSLint rule.

@ajafff
Copy link
Contributor

ajafff commented Jan 25, 2017

@jsynowiec If you need fine grained control for variable name formats, I would recommend one of my custom rules https://github.com/ajafff/tslint-consistent-codestyle#naming-convention
If you scroll down a bit you get to the Examples section where you should find what you need. If you have trouble setting it up for your needs, just leave me an issue over there.

Btw. sorry for shamelessly advertising my own package here.

@pdc
Copy link

pdc commented Jan 27, 2017

I don’t want to relax the naming rules except in this case. It seems we need a new rule which checks that function parameters start with an underscore if and only if they are (a) unused and (b) followed by a parameter that is used. Slightly annoying. :-)

@dead-claudia
Copy link
Author

So, now that I see, I'm thinking:

  • no-unused-variable should be kept. (It makes prototyping on existing code bases easier, since you can just disable it for the unused parameter in question. I personally use the ESLint equivalent frequently)
  • no-invalid-this is fully unnecessary. The correct opt out is just by declaring the type of this, like in function foo(this: any, ...) { ... }.
  • no-switch-case-fallthrough really needs a compiler opt-out, since fallthrough can really avoid code duplication when done right.
  • switch-default should still be kept, since the compiler option isn't a complete replacement. But it'd be worth noting in the docs.

Although, here's a highly relevant TS issue: microsoft/TypeScript#11051

@filipesilva
Copy link
Contributor

Heya, I just wanted to pitch in in favor of keeping no-unused-variable.

My argument is similar to the value of prototyping one. I think it's a different 'level' of checking. You might want a compilation error... or you might want a lint check before commiting.

You might also want to treat different parts of your app with different linting rules, without having a different tsconfig.json.

So I think there's value in having both a linting rule and a compilation setting.

@IllusionMH
Copy link
Contributor

As I understand - there rules won't be used in tslint:recommended configuration and will be removed from this repo.

Maybe it worth to move them to separate repository/package, provide "migration guide" on how to include and enable them in users configuration for those (I expect it to happen on major release)?

@andy-hanson
Copy link
Contributor

Could we deprecate (except no-unused-variable) these for 5.0?

@adidahiya
Copy link
Contributor

@andy-hanson yeah let's do it

@nchen63
Copy link
Contributor

nchen63 commented Mar 29, 2017

keeping these rules for now. See #2422 (comment)

@glen-84
Copy link
Contributor

glen-84 commented Jun 10, 2018

Should this be revisited now that microsoft/TypeScript#9448 was fixed, or wait for microsoft/TypeScript#19139?

I suppose since they require a specific version of TypeScript, the rules should remain until TSLint stops supporting older versions of TypeScript (where they would still be useful).

@dead-claudia
Copy link
Author

dead-claudia commented Jun 10, 2018

If this helps make that judgement, the version @ts-ignore first appeared (2.6) was released late October 2017, and the current version (2.9) was released late May. Currently, what TSLint officially supports is 2.1 or later, but 2.1 was released December of 2016.

Edit: Also, the last time the baseline changed was back in March 2017, where it was updated from 2.0 to 2.1, and the last time before that was in November 2016, where it was updated from 1.7 to 2.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.