Skip to content

chore: de-duplicate lint config #10978

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

chore: de-duplicate lint config #10978

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c2d27d
chore: de-duplicate lint config
43081j Mar 21, 2025
ec2e0ce
Update eslint.config.mjs
kirkwaiblinger Mar 22, 2025
2a12bdd
fix up comment
kirkwaiblinger Mar 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions eslint.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,11 @@ const restrictNamedDeclarations = {
selector: 'ExportNamedDeclaration[declaration=null][source=null]',
};

const vitestFiles = [
'packages/eslint-plugin-internal/tests/**/*.test.{ts,tsx,cts,mts}',
];
const vitestPackages = ['eslint-plugin-internal'];

const vitestFiles = vitestPackages.map(
name => `packages/${name}/tests/**/*.test.{ts,tsx,cts,mts}`,
);
Comment on lines +32 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


export default tseslint.config(
// register all of the plugins up-front
Expand Down Expand Up @@ -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,
Copy link
Contributor Author

@43081j 43081j Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@43081j 43081j Mar 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️ 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

rules: {
'@typescript-eslint/no-empty-function': [
'error',
Expand All @@ -403,6 +404,19 @@ export default tseslint.config(
'@typescript-eslint/no-unsafe-call': 'off',
'@typescript-eslint/no-unsafe-member-access': 'off',
'@typescript-eslint/no-unsafe-return': 'off',
},
},
// jest-specific config
{
files: [
'packages/*/tests/**/*.test.{ts,tsx,cts,mts}',
'packages/*/tests/**/test.{ts,tsx,cts,mts}',
'packages/parser/tests/**/*.{ts,tsx,cts,mts}',
'packages/integration-tests/tools/integration-test-base.ts',
'packages/integration-tests/tools/pack-packages.ts',
],
ignores: vitestFiles,
rules: {
'jest/no-alias-methods': 'error',
'jest/no-deprecated-functions': 'error',
'jest/no-disabled-tests': 'error',
Expand All @@ -419,19 +433,10 @@ export default tseslint.config(
'jest/valid-expect': 'error',
},
},
// test file specific configuration
// vitest-specific configuration
{
files: vitestFiles,
rules: {
'@typescript-eslint/no-empty-function': [
'error',
{ allow: ['arrowFunctions'] },
],
'@typescript-eslint/no-non-null-assertion': 'off',
'@typescript-eslint/no-unsafe-assignment': 'off',
'@typescript-eslint/no-unsafe-call': 'off',
'@typescript-eslint/no-unsafe-member-access': 'off',
'@typescript-eslint/no-unsafe-return': 'off',
'vitest/no-alias-methods': 'error',
'vitest/no-disabled-tests': 'error',
'vitest/no-focused-tests': 'error',
Expand Down