chore: de-duplicate lint config #10978
Conversation
This just simplifies the lint config slightly by doing the following: - Separate vitest and jest rules from regular rules, so we can declare the regular rules once for both - Change `vitestFiles` to be computed in preparation for us moving more packages to vitest (so we don't end up with a bunch of _very similar_ globs)
|
Thanks for the PR, @43081j! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
View your CI Pipeline Execution ↗ for commit 2a12bdd.
☁️ Nx Cloud last updated this comment at |
| @@ -392,7 +394,6 @@ export default tseslint.config( | |||
| 'packages/integration-tests/tools/integration-test-base.ts', | |||
| 'packages/integration-tests/tools/pack-packages.ts', | |||
| ], | |||
| ignores: vitestFiles, | |||
There was a problem hiding this comment.
this meant the various loosening rule options were not applying to vitest tests, but should be. so the previous PR duplicated them below in their own vitest section
moving it into a generic block that applies to all tests fixes this
There was a problem hiding this comment.
TBH I'm kinda -1 on this. It's true that it's DRYer in the moment but the duplication will go away on its own once everything is in vitest, and if we need there to be drift between the jest/vitest parts then this makes it less flexible. So I think as-is makes good sense as a transitional approach, but it's marginal difference either way 🤷 Can go either way if others have opinions.
There was a problem hiding this comment.
🤷♂️ I'm not tied to it enough to mind either way. It's very unlikely we will alter these rules between test frameworks since they're more to do with syntax and casts
Eventually the vitest rules merge into this block, and you remove the jest block. Or your duplicate in both and remove the jest block.
We achieve the same either way, just we duplicate the rules in the latter
| const vitestPackages = ['eslint-plugin-internal']; | ||
|
|
||
| const vitestFiles = vitestPackages.map( | ||
| name => `packages/${name}/tests/**/*.test.{ts,tsx,cts,mts}`, | ||
| ); |
There was a problem hiding this comment.
IMO let's revert this part. It creates merge conflicts on all the vitest PRs for very marginal gain. We can trivially DRY this up once the in-progress PRs are merged.
There was a problem hiding this comment.
Otherwise we're repeating somewhat lengthy globs an increasing amount over time. Hoping they stay in sync
I'd rather fix this now while there's only one instead of doing it later when there's many
|
Given you disagree with both parts of this pr, it seems pretty pointless now I disagree with you on both counts for what it's worth. We shouldn't be duplicating rules, that's just a sign we got it wrong. And we shouldn't be duplicating globs because it increases maintenance burden as we add more and more of what's basically the same glob. It doesn't make sense to do this, it makes sense to have one glob that covers many packages |
|
Closing since I think this is fully superseded by the changes now on main (notably #11078) |
This just simplifies the lint config slightly by doing the following:
vitestFilesto be computed in preparation for us moving more packages to vitest (so we don't end up with a bunch of very similar globs)PR Checklist