Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(linter): Implement no_this_before_super with cfg #2254

Conversation

TzviPM
Copy link
Contributor

@TzviPM TzviPM commented Feb 1, 2024

Implements eslint/no-this-before-super in #479.

Closes #2279

@TzviPM TzviPM self-assigned this Feb 1, 2024
@github-actions github-actions bot added the A-linter Area - Linter label Feb 1, 2024
Copy link

codspeed-hq bot commented Feb 1, 2024

CodSpeed Performance Report

Merging #2254 will not alter performance

Comparing TzviPM:01-29-feat_linter_Implement_no_this_before_super_with_cfg (366452d) with main (d2b304b)

Summary

✅ 17 untouched benchmarks

@TzviPM TzviPM requested a review from Boshen February 1, 2024 14:24
Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

I checked the error span biome and eslint emits, and combining with the general advice on error spans https://oxc-project.github.io/docs/contribute/linter.html#general-advice

Pin point the error message to the shortest code span

We want the user to focus on the problematic code rather than deciphering the error message to identify which part of the code is erroneous.

The best span would be 1 span pointing to the constructor keyword, and the other error span pointing to the this keyword.

 1 │ class A extends B { constructor() { this.c = 0; } }
   ·                     ___________     ____

The second best is to point to the whole method

 1 │ class A extends B { constructor() { this.c = 0; } }
   ·                     _____________________________

@TzviPM TzviPM force-pushed the 01-29-feat_linter_Implement_no_this_before_super_with_cfg branch from e327bff to 564873c Compare February 3, 2024 05:17
Copy link

netlify bot commented Feb 3, 2024

Deploy Preview for oxc canceled.

Name Link
🔨 Latest commit 366452d
🔍 Latest deploy log https://app.netlify.com/sites/oxc/deploys/65bef40f233bec0008c9a3d8

@TzviPM
Copy link
Contributor Author

TzviPM commented Feb 3, 2024

The best span would be 1 span pointing to the constructor keyword, and the other error span pointing to the this keyword.

Do we have a mechanism for this? My concern is that, say there are 2 uses of this before super(), if we highlight the first usage only, then the user fixes that one and then we show the error again with the second usage, it could be confusing. If we are able to have a diagnostic with multiple spans, we can do that here. Or else should we have multiple diagnostics? I'm happy to start by making sure to use the span for the entire method definition node.

@Boshen
Copy link
Member

Boshen commented Feb 3, 2024

Here's an example for multiple spans:

ctx.diagnostic(miette!(
severity = Severity::Warning,
labels = violations,
help = "remove the comma or insert `undefined`",
"eslint(no-sparse-arrays): Unexpected comma in middle of array"
));

Feel free to to pick either one depending on your time availableness.

@TzviPM TzviPM force-pushed the 01-29-feat_linter_Implement_no_this_before_super_with_cfg branch from 564873c to fab10a0 Compare February 4, 2024 02:07
@TzviPM
Copy link
Contributor Author

TzviPM commented Feb 4, 2024

I'm going to submit with the option highlighting the whole method for now and open an issue to make it better. I'd like to explore a bit more what the right approach would be; I have a few ideas.

@TzviPM TzviPM force-pushed the 01-29-feat_linter_Implement_no_this_before_super_with_cfg branch from fab10a0 to 366452d Compare February 4, 2024 02:18
@TzviPM TzviPM requested a review from Boshen February 4, 2024 02:35
@TzviPM
Copy link
Contributor Author

TzviPM commented Feb 4, 2024

I'm going to submit with the option highlighting the whole method for now and open an issue to make it better. I'd like to explore a bit more what the right approach would be; I have a few ideas.

Opened #2290

@Boshen Boshen merged commit 0060d6a into oxc-project:main Feb 4, 2024
26 checks passed
IWANABETHATGUY pushed a commit to IWANABETHATGUY/oxc that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eslint/no-this-before-super
2 participants