Skip to content

Conversation

JoshuaKGoldberg
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Continues #19173.

Updates no-useless-constructor to account for TypeScript syntax as handled in the @typescript-eslint/no-useless-constructor extension rule:

  • constructors marked as protected / private (i.e. marking a constructor as non-public)
  • public constructors when there is no superclass
  • constructors with only parameter properties

Is there anything you'd like reviewers to focus on?

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Mar 19, 2025
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Mar 19, 2025
@github-actions github-actions bot added the rule Relates to ESLint's core rules label Mar 19, 2025
Copy link

netlify bot commented Mar 19, 2025

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 7b2d5c9
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/67daf15781f80f0007e6dbcd

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review March 19, 2025 16:05
@JoshuaKGoldberg JoshuaKGoldberg requested a review from a team as a code owner March 19, 2025 16:05
});

ruleTesterTypeScript.run("no-useless-constructor", rule, {
valid: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make sure to add all the test cases from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got all the ones that have TypeScript-specific keywords, and the existing ones cover the rest of the behavior. Is there anything missing between the two?

I am curious on this: should we include all tests in the TypeScript-specific ones? Just a bare minimum "smoke test" level? All the ones from the general JavaScript one as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant all TS specific + some basic js because the parser is different. Also, can we add loc information assertions on errors?

@snitin315 snitin315 moved this from Needs Triage to Implementing in Triage Mar 21, 2025
@snitin315 snitin315 added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 21, 2025
Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM. I'll leave it open for some time for others to review.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@nzakas nzakas merged commit 557a0d2 into eslint:main Mar 21, 2025
31 checks passed
@github-project-automation github-project-automation bot moved this from Implementing to Complete in Triage Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

3 participants