feat(eslint-plugin)!: recommended-requiring-type-checking config #846

JamesHenry · 2019-08-13T12:07:03Z

BREAKING CHANGE: removed some rules from recommended config

Adds new recommended-requiring-type-checking config
Removes rules from recommended which require type-checking
Updates docs with explanation and usage
Updates config generation scripts
Adds an integration test so that we can ensure there are no regressions in the recommended config not including rules which require type-checking (it would throw if any of the rules required a program)

P.S. TODO in follow up (lower priority because integration test will catch the violations that matter), add some kind of check (maybe we should create a new package for lint rules which only apply to the monorepo and which we don't publish?) to ensure that we correctly mark rules requiring type-checking with requiresTypeChecking: true.

typescript-eslint · 2019-08-13T12:07:05Z

Thanks for the PR, @JamesHenry!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint

BREAKING CHANGE: removed some rules from recommended config

codecov · 2019-08-13T14:07:44Z

Codecov Report

Merging #846 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #846      +/-   ##
==========================================
+ Coverage   94.19%   94.19%   +<.01%     
==========================================
  Files         112      112              
  Lines        4825     4826       +1     
  Branches     1338     1338              
==========================================
+ Hits         4545     4546       +1     
  Misses        160      160              
  Partials      120      120

Impacted Files	Coverage Δ
...ackages/eslint-plugin/src/rules/prefer-includes.ts	`100% <ø> (ø)`	⬆️
...-plugin/src/rules/no-unnecessary-type-arguments.ts	`91.11% <ø> (ø)`	⬆️
...ackages/eslint-plugin/src/rules/no-for-in-array.ts	`100% <ø> (ø)`	⬆️
...slint-plugin/src/rules/no-unnecessary-qualifier.ts	`96.07% <ø> (ø)`	⬆️
...ages/eslint-plugin/src/rules/prefer-regexp-exec.ts	`100% <ø> (ø)`	⬆️
...es/eslint-plugin/src/rules/no-floating-promises.ts	`100% <ø> (ø)`	⬆️
.../eslint-plugin/src/rules/promise-function-async.ts	`100% <ø> (ø)`	⬆️
...-plugin/src/rules/no-unnecessary-type-assertion.ts	`90.78% <ø> (ø)`	⬆️
...int-plugin/src/rules/strict-boolean-expressions.ts	`100% <ø> (ø)`	⬆️
packages/eslint-plugin/src/rules/require-await.ts	`92.1% <ø> (ø)`	⬆️
... and 8 more

bradzacher

Awesome! Everything LGTM!

Two suggestions and then I think this can be merged for the full 2.0 release 🎉 :

Probs should update the configs readme with the new config.
We could potentially delete it, or merge your root plugin readme changes into there?
If we want to keep it, then maybe add a link from the root plugin readme to it so that people can find it easier?

Would be awesome if you could update the doc validator so it supports the new prop you added. Might be a good idea to use this to validate that the boolean flag is set correctly.
(this change would handle your TODO comment)

typescript-eslint/packages/eslint-plugin/tools/validate-docs/validate-table-rules.ts

Lines 103 to 114 in 90b36dd

    
           // quick-and-dirty check to see if it uses parserServices 
        
           // not perfect but should be good enough 
        
           const ruleFileContents = fs.readFileSync( 
        
             path.resolve(__dirname, `../../src/rules/${ruleName}.ts`), 
        
           ); 
        
           const usesTypeInformation = ruleFileContents.includes('getParserServices'); 
        
           validateTableBoolean( 
        
             usesTypeInformation, 
        
             rowNeedsTypeInfo, 
        
             ':thought_balloon:', 
        
             'requiring type information', 
        
           );

I.e.

if (ruleFileContents.includes('getParserServices')) {
  enforce that requiresTypeChecking === true
  table row should have the :thought_balloon:
} else {
  enforce that requiresTypeChecking === false
  table row should not have the :thought_balloon:
}

packages/experimental-utils/src/ts-eslint/Rule.ts

Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>

stale

bradzacher

LGTM! Awesome

JamesHenry requested a review from bradzacher August 13, 2019 12:07

feat(eslint-plugin)!: recommended-requiring-type-checking config

b52298a

BREAKING CHANGE: removed some rules from recommended config

JamesHenry force-pushed the recommended-requiring-type-checking branch from fe15245 to b52298a Compare August 13, 2019 12:13

JamesHenry changed the title ~~feat(eslint-plugin): recommended-requiring-type-checking config~~ feat(eslint-plugin)!: recommended-requiring-type-checking config Aug 13, 2019

fix: name issue and add new config to our own lint setup

e1e107b

bradzacher previously requested changes Aug 13, 2019

View reviewed changes

packages/experimental-utils/src/ts-eslint/Rule.ts Outdated Show resolved Hide resolved

bradzacher added 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge breaking change This change will require a new major version to be released recommended-rules Discussion about recommended rule sets labels Aug 13, 2019

bradzacher added this to the 2.0.0 milestone Aug 13, 2019

bradzacher mentioned this pull request Aug 13, 2019

2.0.0 Breaking Changes #501

Closed

14 tasks

JamesHenry and others added 2 commits August 13, 2019 19:37

Update packages/experimental-utils/src/ts-eslint/Rule.ts

bc5f67d

Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>

chore: extend doc validation and add missing requiresTypeChecking flags

3b0992b

bradzacher approved these changes Aug 13, 2019

View reviewed changes

Merge branch 'master' into recommended-requiring-type-checking

2dc1ba1

JamesHenry merged commit d3470c9 into master Aug 13, 2019

JamesHenry deleted the recommended-requiring-type-checking branch August 13, 2019 19:37

github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin)!: recommended-requiring-type-checking config #846

feat(eslint-plugin)!: recommended-requiring-type-checking config #846

JamesHenry commented Aug 13, 2019 •

edited

Loading

typescript-eslint bot commented Aug 13, 2019

codecov bot commented Aug 13, 2019 •

edited

Loading

bradzacher left a comment •

edited

Loading

bradzacher left a comment

	// quick-and-dirty check to see if it uses parserServices
	// not perfect but should be good enough
	const ruleFileContents = fs.readFileSync(
	path.resolve(__dirname, `../../src/rules/${ruleName}.ts`),
	);
	const usesTypeInformation = ruleFileContents.includes('getParserServices');
	validateTableBoolean(
	usesTypeInformation,
	rowNeedsTypeInfo,
	':thought_balloon:',
	'requiring type information',
	);

feat(eslint-plugin)!: recommended-requiring-type-checking config #846

feat(eslint-plugin)!: recommended-requiring-type-checking config #846

Conversation

JamesHenry commented Aug 13, 2019 • edited Loading

typescript-eslint bot commented Aug 13, 2019

codecov bot commented Aug 13, 2019 • edited Loading

Codecov Report

bradzacher left a comment • edited Loading

Choose a reason for hiding this comment

bradzacher left a comment

Choose a reason for hiding this comment

JamesHenry commented Aug 13, 2019 •

edited

Loading

codecov bot commented Aug 13, 2019 •

edited

Loading

bradzacher left a comment •

edited

Loading