Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from Aug 13, 2019

Conversation

@JamesHenry
Copy link
Member

commented Aug 13, 2019

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

This comment has been minimized.

Copy link

commented Aug 13, 2019

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

@JamesHenry JamesHenry requested a review from bradzacher Aug 13, 2019

feat(eslint-plugin)!: recommended-requiring-type-checking config
BREAKING CHANGE: removed some rules from recommended config

@JamesHenry JamesHenry force-pushed the recommended-requiring-type-checking branch from fe15245 to b52298a Aug 13, 2019

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

@codecov

This comment has been minimized.

Copy link

commented Aug 13, 2019

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
Copy link
Member

left a comment

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)

// 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 Outdated Show resolved Hide resolved

@bradzacher bradzacher added this to the 2.0.0 milestone Aug 13, 2019

@bradzacher bradzacher referenced this pull request Aug 13, 2019
14 of 14 tasks complete

JamesHenry and others added some commits Aug 13, 2019

Update packages/experimental-utils/src/ts-eslint/Rule.ts
Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>
@bradzacher
Copy link
Member

left a comment

LGTM! Awesome

@JamesHenry JamesHenry merged commit d3470c9 into master Aug 13, 2019

7 checks passed

Semantic Pull Request ready to be merged or rebased
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project 94.19% (+<.01%) compared to 90b36dd
Details
typescript-eslint.typescript-eslint Build #20190813.22 succeeded
Details
typescript-eslint.typescript-eslint (Primary code validation and tests) Primary code validation and tests succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_10_x) Run unit tests on other Node.js versions node_10_x succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_8_x) Run unit tests on other Node.js versions node_8_x succeeded
Details

@JamesHenry JamesHenry deleted the recommended-requiring-type-checking branch Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.