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): [max-params] don't count this: void parameter #7696

Merged
merged 11 commits into from
Oct 17, 2023
10 changes: 10 additions & 0 deletions packages/eslint-plugin/docs/rules/max-params.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
description: 'Enforce a maximum number of parameters in function definitions'
---

> 🛑 This file is source code, not the primary documentation location! 🛑
>
> See **https://typescript-eslint.io/rules/max-params** for documentation.

This rule extends the base [`eslint/max-params`](https://eslint.org/docs/rules/max-params) rule.
This version adds support for TypeScript `this` parameter so it won't be counted as a parameter.
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/configs/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ export = {
'@typescript-eslint/lines-around-comment': 'error',
'lines-between-class-members': 'off',
'@typescript-eslint/lines-between-class-members': 'error',
'max-params': 'off',
'@typescript-eslint/max-params': 'error',
'@typescript-eslint/member-delimiter-style': 'error',
'@typescript-eslint/member-ordering': 'error',
'@typescript-eslint/method-signature-style': 'error',
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import keySpacing from './key-spacing';
import keywordSpacing from './keyword-spacing';
import linesAroundComment from './lines-around-comment';
import linesBetweenClassMembers from './lines-between-class-members';
import maxParams from './max-params';
import memberDelimiterStyle from './member-delimiter-style';
import memberOrdering from './member-ordering';
import methodSignatureStyle from './method-signature-style';
Expand Down Expand Up @@ -163,6 +164,7 @@ export default {
'keyword-spacing': keywordSpacing,
'lines-around-comment': linesAroundComment,
'lines-between-class-members': linesBetweenClassMembers,
'max-params': maxParams,
'member-delimiter-style': memberDelimiterStyle,
'member-ordering': memberOrdering,
'method-signature-style': methodSignatureStyle,
Expand Down
97 changes: 97 additions & 0 deletions packages/eslint-plugin/src/rules/max-params.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils';
import type { JSONSchema4 } from '@typescript-eslint/utils/json-schema';

import * as util from '../util';
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
import { getESLintCoreRule } from '../util/getESLintCoreRule';

type FunctionLike =
| TSESTree.FunctionDeclaration
| TSESTree.FunctionExpression
| TSESTree.ArrowFunctionExpression;

type FunctionRuleListener<T extends FunctionLike> = (node: T) => void;

const baseRule = getESLintCoreRule('max-params');

export type Options = util.InferOptionsTypeFromRule<typeof baseRule>;
export type MessageIds = util.InferMessageIdsTypeFromRule<typeof baseRule>;

const schema = [
{
type: 'object',
properties: {
maximum: {
type: 'integer',
minimum: 0,
},
max: {
type: 'integer',
minimum: 0,
},
countVoidThis: {
type: 'boolean',
},
},
additionalProperties: false,
},
] as JSONSchema4[];
Copy link
Contributor Author

@StyleShit StyleShit Sep 27, 2023

Choose a reason for hiding this comment

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

for some reason, I can't get deepMerge to work properly here.

I've tried adding these lines to add support for array overrides, but I'm not sure why it doesn't work:

 if (isObjectNotArray(firstValue) && isObjectNotArray(secondValue)) {
   // object type
   acc[key] = deepMerge(firstValue, secondValue);
+ } else if (Array.isArray(firstValue)) {
+   // array type
+   acc[key] = Object.values(
+     deepMerge({ ...firstValue }, { ...(secondValue as ObjectLike) }),
+   );
  } else {
   // value type
   acc[key] = secondValue;
 }

seems to work fine when isolated though:
https://tsplay.dev/wX1GLN


Anyway... I should've probably still hard-coded the values here since I don't support passing only a number like the base rule does (maybe I should?)

Base schema:
https://github.com/eslint/eslint/blob/main/lib/rules/max-params.js#L30-L53

Copy link
Member

Choose a reason for hiding this comment

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

Extended rules generally directly go off context.options, is that what you're looking for?

const [style, { allowSingleLine } = { allowSingleLine: false }] =
// eslint-disable-next-line no-restricted-syntax -- Use raw options for extended rules.
context.options;

Copy link
Contributor Author

@StyleShit StyleShit Oct 16, 2023

Choose a reason for hiding this comment

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

Nope, I'm talking about baseRule.meta.schema.
I had some issues when trying to add an option to the base schema, the deepMerge function seems to break it.
Is it OK to keep it hard-coded instead of extending the original schema in runtime?

hasSuggestions: baseRule.meta.hasSuggestions,
schema: baseRule.meta.schema,
},

Copy link
Member

Choose a reason for hiding this comment

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

Ah got it - yeah this is fine. Base rules don't change often and many others already hard-code it (e.g. dot-notation). Thanks for asking!


export default util.createRule<Options, MessageIds>({
name: 'max-params',
meta: {
type: 'suggestion',
docs: {
description:
'Enforce a maximum number of parameters in function definitions',
extendsBaseRule: true,
},
schema,
messages: baseRule.meta.messages,
},
defaultOptions: [{ max: 3, countVoidThis: false }],

create(context, [{ countVoidThis }]) {
const baseRules = baseRule.create(context);

if (countVoidThis === true) {
return baseRules;
}

const removeVoidThisParam = <T extends FunctionLike>(node: T): T => {
if (node.params.length === 0) {
return node;
}

const params = [...node.params];
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

if (
params[0] &&
params[0].type === AST_NODE_TYPES.Identifier &&
params[0].name === 'this' &&
params[0].typeAnnotation?.typeAnnotation.type ===
AST_NODE_TYPES.TSVoidKeyword
) {
params.shift();
}

return {
...node,
params,
};
};

const wrapListener = <T extends FunctionLike>(
listener: FunctionRuleListener<T>,
): FunctionRuleListener<T> => {
return (node: T): void => {
listener(removeVoidThisParam(node));
};
};

return {
ArrowFunctionExpression: wrapListener(baseRules.ArrowFunctionExpression),
FunctionDeclaration: wrapListener(baseRules.FunctionDeclaration),
FunctionExpression: wrapListener(baseRules.FunctionExpression),
};
},
});
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/util/getESLintCoreRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ interface RuleMap {
'keyword-spacing': typeof import('eslint/lib/rules/keyword-spacing');
'lines-around-comment': typeof import('eslint/lib/rules/lines-around-comment');
'lines-between-class-members': typeof import('eslint/lib/rules/lines-between-class-members');
'max-params': typeof import('eslint/lib/rules/max-params');
'no-dupe-args': typeof import('eslint/lib/rules/no-dupe-args');
'no-dupe-class-members': typeof import('eslint/lib/rules/no-dupe-class-members');
'no-empty-function': typeof import('eslint/lib/rules/no-empty-function');
Expand Down
104 changes: 104 additions & 0 deletions packages/eslint-plugin/tests/rules/max-params.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import { RuleTester } from '@typescript-eslint/rule-tester';

import rule from '../../src/rules/max-params';

const ruleTester = new RuleTester({
parser: '@typescript-eslint/parser',
});

ruleTester.run('max-params', rule, {
valid: [
'function foo() {}',
'const foo = function () {};',
'const foo = () => {};',
'function foo(a) {}',
`
class Foo {
constructor(a) {}
}
`,
`
class Foo {
method(this: void, a, b, c) {}
}
`,
`
class Foo {
method(this: Foo, a, b) {}
}
`,
{
code: 'function foo(a, b, c, d) {}',
options: [{ max: 4 }],
},
{
code: 'function foo(a, b, c, d) {}',
options: [{ maximum: 4 }],
},
{
code: `
class Foo {
method(this: void) {}
}
`,
options: [{ max: 0 }],
},
{
code: `
class Foo {
method(this: void, a) {}
}
`,
options: [{ max: 1 }],
},
{
code: `
class Foo {
method(this: void, a) {}
}
`,
options: [{ max: 2, countVoidThis: true }],
},
],
invalid: [
{ code: 'function foo(a, b, c, d) {}', errors: [{ messageId: 'exceed' }] },
{
code: 'const foo = function (a, b, c, d) {};',
errors: [{ messageId: 'exceed' }],
},
{
code: 'const foo = (a, b, c, d) => {};',
errors: [{ messageId: 'exceed' }],
},
{
code: 'const foo = a => {};',
options: [{ max: 0 }],
errors: [{ messageId: 'exceed' }],
},
{
code: `
class Foo {
method(this: void, a, b, c, d) {}
}
`,
errors: [{ messageId: 'exceed' }],
},
{
code: `
class Foo {
method(this: void, a) {}
}
`,
options: [{ max: 1, countVoidThis: true }],
errors: [{ messageId: 'exceed' }],
},
{
code: `
class Foo {
method(this: Foo, a, b, c) {}
}
`,
errors: [{ messageId: 'exceed' }],
},
],
});
18 changes: 18 additions & 0 deletions packages/eslint-plugin/typings/eslint-rules.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,24 @@ declare module 'eslint/lib/rules/keyword-spacing' {
export = rule;
}

declare module 'eslint/lib/rules/max-params' {
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';

const rule: TSESLint.RuleModule<
'exceed',
(
| { max: number; countVoidThis?: boolean }
| { maximum: number; countVoidThis?: boolean }
)[],
{
FunctionDeclaration(node: TSESTree.FunctionDeclaration): void;
FunctionExpression(node: TSESTree.FunctionExpression): void;
ArrowFunctionExpression(node: TSESTree.ArrowFunctionExpression): void;
}
>;
export = rule;
}

declare module 'eslint/lib/rules/no-dupe-class-members' {
import type { TSESLint, TSESTree } from '@typescript-eslint/utils';

Expand Down