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 calledprocessPrivateIdentifier such that it could be called from different kinds of nodes.
Then, I added thePropertyDefinition selector.
Finally, I added some basic tests. Since the logic is directly reused from the base rule, more Typescript-specific tests are probably superfluous.

Zamiell added6 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

Copy link

Contributor

typescript-eslintbot commentedNov 2, 2024

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.

Copy link

netlifybot commentedNov 2, 2024•
edited
Loading

✅ Deploy Preview fortypescript-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 yourNetlify site configuration.

Copy link

nx-cloudbot commentedNov 2, 2024•
edited
Loading

View yourCI Pipeline Execution ↗ for commit684c009.

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 at2024-12-31 21:07:13 UTC

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

1e04837

Copy link

ContributorAuthor

Zamiell commentedNov 2, 2024

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 2npm error path D:\Repositories\typescript-eslint\packages\eslint-pluginnpm error workspace @typescript-eslint/eslint-plugin@8.12.2npm error location D:\Repositories\typescript-eslint\packages\eslint-pluginnpm error command failednpm 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 requested changes

Nov 4, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

A good start!

packages/eslint-plugin/tests/rules/no-unused-private-class-members.test.tsShow resolvedHide resolved

packages/eslint-plugin/src/rules/no-unused-private-class-members.tsShow resolvedHide resolved

packages/eslint-plugin/docs/rules/no-unused-private-class-members.mdxShow resolvedHide resolved

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

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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 frommain and re-building. 🤷

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

#10288 ✨

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

- 429: Unable to connect to Nx Cloud.

Copy link

Member

Choose a reason for hiding this comment

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

429 Too Many Requests

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

Copy link

ContributorAuthor

ZamiellNov 9, 2024•
edited
Loading

Choose a reason for hiding this comment

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

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:

I am not sure what they mean though.

Copy link

Member

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

ZamiellNov 9, 2024•
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

Member

JoshuaKGoldbergNov 10, 2024

Choose a reason for hiding this comment

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

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.