Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): noUselessConstructor #4385

Merged
merged 1 commit into from
Apr 21, 2023
Merged

feat(rome_js_analyze): noUselessConstructor #4385

merged 1 commit into from
Apr 21, 2023

Conversation

Conaclos
Copy link
Contributor

Summary

This implements no-useless-constructor with two differences:

  • We do not detectect useless constructors with a super-call that spreads the legacy arguments.
  • We report all empty constructor marked with the public modifier. In contrast, TypeScript ESLint does not report empty constructor marked with the public modifier in a class that inherits of another class.

Test Plan

ESLint tests included.

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Apr 18, 2023

Deploy Preview for docs-rometools failed.

Built without sensitive environment variables

Name Link
🔨 Latest commit c4721fc
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64429562c20c66000840cfc9

@github-actions github-actions bot added A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Parser Area: parser A-Project Area: project configuration and loading labels Apr 18, 2023
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

You might want to take in consideration TypeScript constructor overload. Some frameworks use that feature

@Conaclos
Copy link
Contributor Author

Conaclos commented Apr 18, 2023

You might want to take in consideration TypeScript constructor overload. Some frameworks use that feature

In the provided link, the code uses a parameter property to initialize the injected service.
The rule does not report empty constructors with at least one parameter property.

@denbezrukov
Copy link
Contributor

denbezrukov commented Apr 18, 2023

I was wondering if you could implement a fix for this rule.
Could we remove the constructor?

What do you about a case when we have comments inside the body?

@ematipico
Copy link
Contributor

ematipico commented Apr 18, 2023

You might want to take in consideration TypeScript constructor overload. Some frameworks use that feature

In the povided link, the code uses a parameter property to initialize the injected service.
The rule does not report empty constructors with at least one parameter property.

Cool, would you mind add a test case for that?

@Conaclos
Copy link
Contributor Author

Conaclos commented Apr 19, 2023

I was wondering if you could implement a fix for this rule. Could we remove the constructor?

What do you about a case when we have comments inside the body?

What I suggest:

  • safe fix when there are no comments in the body or jsdoc before the constructor,
  • suggested fix otherwise.

What do you think?

EDIT: I applied my suggestion. Waiting for your input :)

@Conaclos Conaclos requested a review from ematipico April 19, 2023 11:54
@Conaclos
Copy link
Contributor Author

Cool, would you mind add a test case for that?

Done :)

@denbezrukov
Copy link
Contributor

denbezrukov commented Apr 19, 2023

  • safe fix when there are no comments in the body or jsdoc before the constructor

I like that!

@Conaclos Conaclos requested a review from ematipico April 21, 2023 13:54
@ematipico ematipico merged commit 10ec2ab into rome:main Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Parser Area: parser A-Project Area: project configuration and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants