Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Enable more TSLint rules from core #491

Closed
4 of 17 tasks
JoshuaKGoldberg opened this issue Aug 13, 2018 · 7 comments
Closed
4 of 17 tasks

Enable more TSLint rules from core #491

JoshuaKGoldberg opened this issue Aug 13, 2018 · 7 comments
Labels
Difficulty: Easy Someone with little to no experience in TSLint should be able to send a pull request for this issue. Domain: Tooling Repository tasks around improving source tooling. Good First Issue 🙌 Howdy, neighbor! 💀 R.I.P. 💀 Status: Accepting PRs

Comments

@JoshuaKGoldberg
Copy link

JoshuaKGoldberg commented Aug 13, 2018

Keeping a pending list on this issue of rules that would be nice to enable internally for tslint-microsoft-contrib. For each rule, it'd be great to see a single PR that enables it in tslint.json and fixes any resultant failures in source code.

  • ban-types
  • deprecation
  • jsdoc-format
  • member-access
  • no-any
  • no-boolean-literal-compare
  • no-inferrable-types
  • no-internal-module / no-namespace
  • no-non-null-assertion
  • no-unbound-method
  • no-unsafe-any
  • prefer-for-of
  • prefer-function-over-method
  • prefer-readonly
  • restrict-plus-operands
  • strict-boolean-expressions
  • strict-type-predicates
@JoshuaKGoldberg JoshuaKGoldberg added Status: Accepting PRs Difficulty: Hard Only someone with heavy experience in TSLint should be able to send a pull request for this issue. Domain: Tooling Repository tasks around improving source tooling. labels Aug 13, 2018
@reduckted
Copy link
Contributor

It looks like the current grunt task doesn't specify the TypeScript project, which means no type-checked rules are used. There are three warnings that pop up:

Running "tslint:prod" (tslint) task
Warning: The 'no-cookies' rule requires type information.
Warning: The 'no-use-before-declare' rule requires type information.
Warning: The 'prefer-readonly' rule requires type information.

If you run with type checking, the 'no-use-before-declare' and 'prefer-readonly' rules produce quite a lot of violations.

@JoshuaKGoldberg
Copy link
Author

Hmm, that was probably set up before TSLint even had type checking. +1.

@reduckted
Copy link
Contributor

It also looks like the grunt task isn't linting everything that it could. The no-null-keyword rule has been turned on, and running npm test results in everything passing, but if you run tslint directly, there are a handful of cases where the no-null-keyword rule finds problems. In any case, those instances of using null have been fixed indirectly by #570, and the linting via grunt will no doubt end up being fixed in #484.

@IllusionMH
Copy link
Contributor

IllusionMH commented Oct 21, 2018

Yeah, looks like grunt-tslint is too old to support extends block in configuration (or not configured properly to do so). I've had problem with couple of other lint errors during migration.
And indeed migration to npm scripts (TSLint CLI) fixes this problem (extends ignored in config for tests).

@reduckted
Copy link
Contributor

@JoshuaKGoldberg, not sure if you want to track it here as well, but I've just disabled no-use-before-declare (tracked at #603) as a result of turning on type-checking in #604.

@JoshuaKGoldberg JoshuaKGoldberg added Difficulty: Medium People with non-trivial experience in TSLint should be able to send a pull request for this issue. and removed Difficulty: Hard Only someone with heavy experience in TSLint should be able to send a pull request for this issue. labels Oct 29, 2018
JoshuaKGoldberg pushed a commit that referenced this issue Dec 13, 2018
Includes @IllusionMH's changes to `exportNameRule.ts` from #666. This shouldn't be merged in until that PR is.

Continues #491.
@JoshuaKGoldberg JoshuaKGoldberg added Good First Issue 🙌 Howdy, neighbor! Difficulty: Easy Someone with little to no experience in TSLint should be able to send a pull request for this issue. and removed Difficulty: Medium People with non-trivial experience in TSLint should be able to send a pull request for this issue. labels Dec 26, 2018
@JoshuaKGoldberg
Copy link
Author

Now that the tooling is a bit more fleshed out, marking this as a good first issue and easy difficulty. Updating the OP to be a bit more clear.

apawast pushed a commit to lupine86/tslint-microsoft-contrib that referenced this issue Feb 26, 2019
Includes @IllusionMH's changes to `exportNameRule.ts` from microsoft#666. This shouldn't be merged in until that PR is.

Continues microsoft#491.
@JoshuaKGoldberg
Copy link
Author

💀 It's time! 💀

TSLint is being deprecated; therefore, it and tslint-microsoft-contrib are no longer accepting pull requests for major new changes or features. See palantir/tslint#4534. 😱

If you'd like to see this change implemented, you have two choices:

  • Recommended: Check if this is available in ESLint + typescript-eslint
  • Not Recommended: Fork TSLint or tslint-microsoft-contrib locally 🤷‍♂️

👋 It was a pleasure open sourcing with you!

If you believe this message was posted here in error, please comment so we can re-open the issue!

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Difficulty: Easy Someone with little to no experience in TSLint should be able to send a pull request for this issue. Domain: Tooling Repository tasks around improving source tooling. Good First Issue 🙌 Howdy, neighbor! 💀 R.I.P. 💀 Status: Accepting PRs
Projects
None yet
Development

No branches or pull requests

3 participants