feat(eslint-plugin): [max-params] don't count `this: void` parameter #7696

StyleShit · 2023-09-27T12:29:31Z

typescript-eslint · 2023-09-27T12:29:34Z

Thanks for the PR, @StyleShit!

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.

netlify · 2023-09-27T12:29:35Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`b69113a`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/652e90e056d41c00087fcaed
😎 Deploy Preview	https://deploy-preview-7696--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 96 (🟢 up 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2023-09-27T12:32:05Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b69113a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 19 targets

Sent with 💌 from NxCloud.

StyleShit · 2023-09-27T12:55:14Z

packages/eslint-plugin/src/rules/max-params.ts

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


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

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

typescript-eslint/packages/eslint-plugin/src/rules/brace-style.ts

Lines 30 to 32 in ca1abb5

const [style, { allowSingleLine } = { allowSingleLine: false }] =

// eslint-disable-next-line no-restricted-syntax -- Use raw options for extended rules.

context.options;

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?

typescript-eslint/packages/eslint-plugin/src/rules/brace-style.ts

Lines 25 to 27 in ca1abb5

hasSuggestions: baseRule.meta.hasSuggestions,

schema: baseRule.meta.schema,

},

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!

JoshuaKGoldberg

Looks great to me! Nice and straightforward, I like it. 🙂

Left a couple of suggestions - what do you think?

packages/eslint-plugin/docs/rules/max-params.md

packages/eslint-plugin/src/rules/max-params.ts

JoshuaKGoldberg · 2023-10-13T16:07:12Z

packages/eslint-plugin/src/rules/max-params.ts

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


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

typescript-eslint/packages/eslint-plugin/src/rules/brace-style.ts

Lines 30 to 32 in ca1abb5

const [style, { allowSingleLine } = { allowSingleLine: false }] =

// eslint-disable-next-line no-restricted-syntax -- Use raw options for extended rules.

context.options;

…/max-params

…x-params

packages/eslint-plugin/src/rules/max-params.ts

JoshuaKGoldberg · 2023-10-17T13:39:33Z

packages/eslint-plugin/src/rules/max-params.ts

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


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!

JoshuaKGoldberg

Looks great, thanks for iterating on this! Great work 🚀

Just a couple of small things that I can touch up before merging.

packages/eslint-plugin/src/rules/max-params.ts

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

feat(eslint-plugin): [max-params] don't count this: void parameter

d542963

Closes typescript-eslint#7538

hard-code schema

59f2f37

StyleShit commented Sep 27, 2023

View reviewed changes

StyleShit added 3 commits September 27, 2023 15:56

refactor

f9c5958

lint

23c4ce7

snapshot

f613530

StyleShit marked this pull request as ready for review September 27, 2023 18:00

Merge branch 'main' into feature/max-params

1f82e3a

JoshuaKGoldberg requested changes Oct 13, 2023

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Oct 13, 2023

StyleShit added 3 commits October 16, 2023 20:10

wip

9b258f7

Merge remote-tracking branch 'origin/feature/max-params' into feature…

ef0a019

…/max-params

Merge remote-tracking branch 'typescript-eslint/main' into feature/ma…

80b6fc1

…x-params

StyleShit requested a review from JoshuaKGoldberg October 16, 2023 17:14

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 16, 2023

JoshuaKGoldberg reviewed Oct 17, 2023

View reviewed changes

JoshuaKGoldberg previously approved these changes Oct 17, 2023

View reviewed changes

packages/eslint-plugin/src/rules/max-params.ts Outdated Show resolved Hide resolved

Update packages/eslint-plugin/src/rules/max-params.ts

cfc6c29

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

StyleShit dismissed JoshuaKGoldberg’s stale review via cfc6c29 October 17, 2023 13:46

Small PR nits, don't mind me, it's great either way

b69113a

JoshuaKGoldberg merged commit 6398d3f into typescript-eslint:main Oct 17, 2023
46 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [max-params] don't count `this: void` parameter #7696

feat(eslint-plugin): [max-params] don't count `this: void` parameter #7696

StyleShit commented Sep 27, 2023

typescript-eslint bot commented Sep 27, 2023

netlify bot commented Sep 27, 2023 •

edited

nx-cloud bot commented Sep 27, 2023 •

edited

StyleShit Sep 27, 2023 •

edited

JoshuaKGoldberg Oct 13, 2023

StyleShit Oct 16, 2023 •

edited

JoshuaKGoldberg Oct 17, 2023

JoshuaKGoldberg left a comment

JoshuaKGoldberg Oct 13, 2023

JoshuaKGoldberg Oct 17, 2023

JoshuaKGoldberg left a comment

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

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

feat(eslint-plugin): [max-params] don't count this: void parameter #7696

feat(eslint-plugin): [max-params] don't count this: void parameter #7696

Conversation

StyleShit commented Sep 27, 2023

typescript-eslint bot commented Sep 27, 2023

netlify bot commented Sep 27, 2023 • edited

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Sep 27, 2023 • edited

☁️ Nx Cloud Report

StyleShit Sep 27, 2023 • edited

Choose a reason for hiding this comment

JoshuaKGoldberg Oct 13, 2023

Choose a reason for hiding this comment

StyleShit Oct 16, 2023 • edited

Choose a reason for hiding this comment

JoshuaKGoldberg Oct 17, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Oct 13, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg Oct 17, 2023

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

feat(eslint-plugin): [max-params] don't count `this: void` parameter #7696

feat(eslint-plugin): [max-params] don't count `this: void` parameter #7696

netlify bot commented Sep 27, 2023 •

edited

nx-cloud bot commented Sep 27, 2023 •

edited

StyleShit Sep 27, 2023 •

edited

StyleShit Oct 16, 2023 •

edited