feat(eslint-plugin): [lines-around-comment] add extension rule

[chbdetta]

PR Checklist

• Addresses an existing open issue: fixes [lines-around-comment] Does not respect interfaces #1150
• That issue was marked as accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

port the eslint/lines-around-comment rule and support interface and type literals

Options

in addition to the base rule options, this adds:

allowInterfaceStart?: boolean;
allowInterfaceEnd?: boolean;
allowTypeStart?: boolean;
allowTypeEnd?: boolean;
allowEnumStart?: boolean;
allowEnumEnd?: boolean;
allowModuleStart?: boolean;
allowModuleEnd?: boolean;

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 75bf6ea. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 47 targets

• Node 18 - nx test eslint-plugin
• Node 14 - nx test eslint-plugin --coverage=false
• nx run-many --target=lint --parallel
• Node 18 - nx test type-utils
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test visitor-keys
• Node 18 - nx test typescript-estree
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test utils
• Node 18 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=typecheck --parallel
• Node 14 - nx test visitor-keys --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test ast-spec
• Node 14 - nx test parser --coverage=false
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test ast-spec --coverage=false
• Node 18 - nx test parser
• Node 18 - nx test scope-manager
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx test eslint-plugin-internal
• nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test eslint-plugin-tslint --coverage=false
• Node 14 - nx test eslint-plugin-internal --coverage=false
• Node 18 - nx test eslint-plugin-tslint
• nx run-many --target=build --parallel --exclude website
• Node 14 - nx test type-utils --coverage=false
• Node 14 - nx test utils --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test scope-manager --coverage=false
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 18 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx test typescript-estree --coverage=false
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• Node 14 - nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website
• nx run-many --target=build --parallel --exclude website

---

Sent with 💌 from NxCloud.

[typescript-eslint]

Thanks for the PR, @chbdetta!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	75bf6ea
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/63d06dff2fcfa900090ed05e
😎 Deploy Preview	https://deploy-preview-5327--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[codecov]

Codecov Report

Merging #5327 (75bf6ea) into main (67706e7) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5327      +/-   ##
==========================================
+ Coverage   91.36%   91.44%   +0.07%     
==========================================
  Files         368      369       +1     
  Lines       12592    12709     +117     
  Branches     3707     3745      +38     
==========================================
+ Hits        11505    11622     +117     
  Misses        772      772              
  Partials      315      315              

Flag	Coverage Δ
unittest	91.44% <100.00%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ckages/eslint-plugin/src/util/getESLintCoreRule.ts	75.00% <ø> (ø)
...es/eslint-plugin/src/rules/lines-around-comment.ts	100.00% <100.00%> (ø)

[pkuczynski]

@JoshuaKGoldberg and @bradzacher any chance to merge this in?

[bradzacher]

@pkuczynski dont ping maintainers directly. It's poor etiquette to try and bump priorities by doing this. It doesn't change our priorities, and just spams people anyone watching this PR as well as the tagged.

We have a documented review process and this PR is in the queue.

We are volunteers, and have little time to allocate to everything.
We'll get to it when we have the time to allocate to review a 4,000 line PR.

[pkuczynski]

Dear @bradzacher apologies for my impatience. I know how much work it is to manage popular open source package, as I am maintainer myself. I just noticed quite some activity in other PRs beings commented and merged, and this felt a bit abandoned, so indeed I wanted to bring a little bit of your attention...

[JoshuaKGoldberg]

Ok! Thanks for sending this in and for all the thorough tests ported over from ESLint. This is a really great start. ❤️

But: I think we can reduce the lines of coded added by a bunch by reusing the base rule (rules.Program()).

[JoshuaKGoldberg]

The base rule already covers arrays, classes, and basically everything else that's not an interface or type. So you should be able to do what other rule extensions do and call the base rule to reuse its logic:

    const rules = baseRule.create(context, [options]);

    return {
      Program(): void {
        rules.Program();

        comments.forEach(...);

I prototyped this locally and it looks to get pretty much all the existing test cases. Is there any reason why this strategy wouldn't work?

It'd be a real shame to copy & paste so much core ESLint rule logic. We'd then have to maintain it all over time.

[chbdetta]

Sorry for the late response! I finally have time to take a look at this.

Because the base rule will always report false positive errors on TS codes (which is what we're fixing here), to reuse the Program() from the base rule, I have to create a custom context.report function that silences errors caused by TS-codes and feed it to the base rule. (Please let me know if there is a proper way to silence an error report)

By reusing the base rule, I can now skip checking non-TS codes since they are already handled by the base rule.

Changes in 30825c6

[prescience-data]

Woo! Amazing to see a PR on this, I was about to start one until I noticed this 🥳

[bradzacher]

there are a large number of uncovered lines in the rule - could you please add some more test cases to get the coverage higher?

[JoshuaKGoldberg]

@viveleroi the status is what it looks like: we posted some suggestions and are waiting on @chbdetta to address them.

Normally I'd link to our Contributing guide to indicate that we normally ask folks to not post in PRs asking for updates. But I can see why you'd post now - it's been a month with no updates. I just updated #5942 to also mention PRs.

[JoshuaKGoldberg]

👋 @chbdetta just checking in, do you still have the time to work on this? No worries if not - we can always close the PR and let the issue be open for anybody else who wants to send a PR.

[chbdetta]

@JoshuaKGoldberg Thanks for bumping this! I was able to look at this again. I ran into some trouble getting the coverage report locally after merging the latest main, do you know how to resolve this?

Running coverage on untested files...Failed to collect coverage from /workspace/typescript-eslint/packages/eslint-plugin/src/configs/strict.ts
ERROR: 
  x Export assignment cannot be used when targeting ECMAScript modules. Consider using `export default` or another module format instead.
    ,-[/workspace/typescript-eslint/packages/eslint-plugin/src/configs/strict.ts:5:1]

[JoshuaKGoldberg]

...huh, I could swear I've responded to that message about testing coverage failure somewhere else. But can't find it. Anyway:

• What Node version are you running with locally? I don't reproduce this issue on Node 16 or 18
• You can always check the codecov report online: https://app.codecov.io/gh/typescript-eslint/typescript-eslint/compare/5327

[chbdetta]

@JoshuaKGoldberg @bradzacher please review the PR again. I delegate most of the non-TS cases to the base rule so I only keep the TS-related test cases.

[JoshuaKGoldberg]

All right, I poked at this as much as I can and it looks good to me! Thanks for sending this in & waiting for the review @chbdetta. 🔥

@bradzacher I'll wait another week or two for your review if you have time since you've been looking at extension rules a bunch. Otherwise I'll be good to merge in.