feat(eslint-plugin): [no-unused-private-class-members] new extension rule #10265

Zamiell · 2024-11-02T11:34:18Z

PR Checklist

Addresses an existing open issue: fixes Rule proposal: no-unused-private-class-members #4571
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

First I copied the base rule from ESLint.
Next, I converted it to TypeScript + made it pass the linter + ensured that all tests were still passing.
Then, I refactored the main logic into a separate function called processPrivateIdentifier such that it could be called from different kinds of nodes.
Then, I added the PropertyDefinition selector.
Finally, I added some basic tests. Since the logic is directly reused from the base rule, more Typescript-specific tests are probably superfluous.

typescript-eslint · 2024-11-02T11:34:22Z

Thanks for the PR, @Zamiell!

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 · 2024-11-02T11:34:34Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`684c009`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/672f4dfb4818a300081261e5
😎 Deploy Preview	https://deploy-preview-10265--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 7 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 · 2024-11-02T11:35:12Z

View your CI Pipeline Execution ↗ for commit 684c009.

Command	Status	Duration	Result
`nx run-many --target=build --exclude website --...`	✅ Succeeded	54s	View ↗
`nx run-many --target=clean`	✅ Succeeded	11s	View ↗

☁️ Nx Cloud last updated this comment at 2024-12-31 21:07:13 UTC

Zamiell · 2024-11-02T12:28:55Z

The build is failing:

james@1qaz MSYS /d/Repositories/typescript-eslint/packages/eslint-plugin (no-unused-private-class-members)
$ npm run build

> @typescript-eslint/eslint-plugin@8.12.2 build
> tsc -b tsconfig.build.json

../typescript-estree/src/convert.ts:2253:11 - error TS2367: This comparison appears to be unintentional because the types 'SyntaxKind.Identifier' and 'SyntaxKind.StringLiteral' have no overlap.

2253           local.kind === SyntaxKind.StringLiteral &&
               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 1 error.

npm error Lifecycle script `build` failed with error:
npm error code 2
npm error path D:\Repositories\typescript-eslint\packages\eslint-plugin
npm error workspace @typescript-eslint/eslint-plugin@8.12.2
npm error location D:\Repositories\typescript-eslint\packages\eslint-plugin
npm error command failed
npm error command C:\Windows\system32\cmd.exe /d /s /c tsc -b tsconfig.build.json

However, this seems unrelated to my PR. Please advise.

JoshuaKGoldberg

A good start!

packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts

packages/eslint-plugin/src/rules/no-unused-private-class-members.ts

packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdx

packages/eslint-plugin/src/rules/no-unused-private-class-members.ts

JoshuaKGoldberg · 2024-11-04T16:30:14Z

packages/eslint-plugin/src/rules/no-unused-private-class-members.ts

+      // }
+      // ```
+      PrivateIdentifier(node): void {
+        processPrivateIdentifier(node);


Threading #10265 (comment): I don't get that error locally. But I do get what Nx is getting (https://cloud.nx.app/runs/lsSb3ZQa0g?utm_source=pull-request&utm_medium=comment):

src/rules/no-unused-private-class-members.ts(215,34): error TS2345: Argument of type 'PrivateIdentifier' is not assignable to parameter of type 'Identifier'. Type 'PrivateIdentifier' is missing the following properties from type 'Identifier': decorators, optional, typeAnnotation

Maybe a caching issue? I'd suggest merging from main and re-building. 🤷

I merged with main and now I am getting this Netlify error: https://app.netlify.com/sites/typescript-eslint/deploys/67295a26e190f200080d6053

Furthermore, it looks like "docs.test.ts" has a lint error, but that is unrelated to my PR.

I merged with main, but CI is now getting this error:

- 429: Unable to connect to Nx Cloud.

429 Too Many Requests

It'll probably ease up on you if you push less.

If you are implying that I am committing too much, I thought it was best-practice to put each PR fix that was brought up in a PR review thread by other people into a separate commit, which I did.

Anyways, I just merged from main again. Now, I get these two CI errors:

https://cloud.nx.app/runs/WZCMfyPxRY/task/typescript-eslint%3Atest

https://cloud.nx.app/runs/KCqe66ahXv/task/eslint-plugin%3Atest

I am not sure what they mean though.

best-practice to put each PR fix that was brought up in a PR review thread by other people into a separate commit

I don't subscribe to that best practice 😄. It can be nice to have granular commits when there are tricky things being discussed. But there's no real utility most of the time.

The downside is that a lot of CI churn gets added to the repo. Which is never good: almost all repos are either on a rate-limited free plan (like we are) or a pay-for-bandwidth plan ($$$).

Now, I get these two CI errors:

You've got some test failures, have you tried reading through the test files mentioned in them? I haven't looked deeply but if you've got specific questions I can.

You've got some test failures

Right, but the part I am confused about is that the test failures are in files that I haven't modified / not part of this PR. Do I have to add the new rule to one of the existing configs or something?

That's what I'm trying to hint at with my questions: did you read the tests that are failing? The test names, the test failures, the code they touch?

Ideally, the repo's docs and tests should be set up so that you can debug through these and figure this out on your own. If it's not clear from them then that would be something we'd want to work on.

I'm not intentionally trying to be obtuse or unhelpful. But these are things that I think as someone who's got a bunch of PRs merged into this repo (❤️) you're equipped to debug.

packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts

JoshuaKGoldberg · 2024-12-02T14:58:09Z

👋 Hey @Zamiell! Just checking in, is this still something you have time for? No worries if not - I just don't want to leave it hanging.

JoshuaKGoldberg · 2024-12-31T21:05:15Z

👋 ping @Zamiell - this rule would be great, and I'm free to answer specific questions if you have any?

Zamiell · 2025-01-10T00:13:14Z

yeah i still plan to work on this when i get some more free time, i have been very sick lately

JoshuaKGoldberg · 2025-01-10T13:48:29Z

❤️ hope you feel better!

Removing stale and setting as a draft for now. If this is still pending in February I'd say we should close it out to un-block the issue.

JoshuaKGoldberg · 2025-03-02T07:02:19Z

Closing out to un-block the issue, as planned. Cheers!

Zamiell added 6 commits November 2, 2024 06:38

feat: implement original rule without any other modifications

91f3406

chore: document data structure

b2f0126

update

e37fd8b

update

819b8a5

update

c621697

update

389e1f2

Merge branch 'main' into no-unused-private-class-members

1e04837

JoshuaKGoldberg requested changes Nov 4, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Nov 4, 2024

JoshuaKGoldberg reviewed Nov 4, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.ts Outdated Show resolved Hide resolved

Zamiell added 10 commits November 4, 2024 18:26

update

e82d5f5

update

57369ab

update

12c2c6d

update

76f1693

update

b8231a9

Merge branch 'main' into no-unused-private-class-members

2e4b0e8

update

d430b17

Merge branch 'main' into no-unused-private-class-members

597b25d

update

45320ab

Merge branch 'main' into no-unused-private-class-members

684c009

JoshuaKGoldberg added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Dec 31, 2024

JoshuaKGoldberg removed the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Jan 10, 2025

JoshuaKGoldberg marked this pull request as draft January 10, 2025 13:48

JoshuaKGoldberg mentioned this pull request Jan 10, 2025

feat(eslint-plugin): [await-thenable] check Promise.all #10282

Closed

3 tasks

JoshuaKGoldberg closed this Mar 2, 2025

github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [no-unused-private-class-members] new extension rule #10265

feat(eslint-plugin): [no-unused-private-class-members] new extension rule #10265

Zamiell commented Nov 2, 2024 •

edited

Loading

typescript-eslint bot commented Nov 2, 2024

netlify bot commented Nov 2, 2024 •

edited

Loading

nx-cloud bot commented Nov 2, 2024 •

edited

Loading

Zamiell commented Nov 2, 2024

JoshuaKGoldberg left a comment

JoshuaKGoldberg Nov 4, 2024

Zamiell Nov 4, 2024

Zamiell Nov 4, 2024

JoshuaKGoldberg Nov 6, 2024

Zamiell Nov 6, 2024

JoshuaKGoldberg Nov 6, 2024

Zamiell Nov 9, 2024 •

edited

Loading

JoshuaKGoldberg Nov 9, 2024

Zamiell Nov 9, 2024 •

edited

Loading

JoshuaKGoldberg Nov 10, 2024

JoshuaKGoldberg commented Dec 2, 2024

JoshuaKGoldberg commented Dec 31, 2024

Zamiell commented Jan 10, 2025

JoshuaKGoldberg commented Jan 10, 2025

JoshuaKGoldberg commented Mar 2, 2025

feat(eslint-plugin): [no-unused-private-class-members] new extension rule #10265

feat(eslint-plugin): [no-unused-private-class-members] new extension rule #10265

Conversation

Zamiell commented Nov 2, 2024 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Nov 2, 2024

netlify bot commented Nov 2, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Nov 2, 2024 • edited Loading

Zamiell commented Nov 2, 2024

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zamiell Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zamiell Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Dec 2, 2024

JoshuaKGoldberg commented Dec 31, 2024

Zamiell commented Jan 10, 2025

JoshuaKGoldberg commented Jan 10, 2025

JoshuaKGoldberg commented Mar 2, 2025

Zamiell commented Nov 2, 2024 •

edited

Loading

netlify bot commented Nov 2, 2024 •

edited

Loading

nx-cloud bot commented Nov 2, 2024 •

edited

Loading

Zamiell Nov 9, 2024 •

edited

Loading

Zamiell Nov 9, 2024 •

edited

Loading