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

chbdetta · 2022-07-09T07:51:17Z

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 · 2022-07-09T07:51:19Z

☁️ 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

Sent with 💌 from NxCloud.

typescript-eslint · 2022-07-09T07:51:20Z

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 · 2022-07-09T07:51:24Z

✅ 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 · 2022-07-09T08:04:47Z

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 · 2022-07-28T10:48:32Z

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

bradzacher · 2022-07-28T11:09:29Z

@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 · 2022-07-29T05:46:09Z

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()).

packages/eslint-plugin/src/rules/lines-around-comment.ts

JoshuaKGoldberg · 2022-08-07T21:42:37Z

packages/eslint-plugin/src/rules/lines-around-comment.ts

+    /**
+     * Returns whether or not comments are at the array start or not.
+     */
+    function isCommentAtArrayStart(token: TSESTree.Token): boolean {


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.

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

packages/eslint-plugin/src/rules/lines-around-comment.ts

prescience-data · 2022-09-23T00:19:00Z

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

- waiting on typescript-eslint/typescript-eslint#5327

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?

packages/eslint-plugin/src/rules/lines-around-comment.ts

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

JoshuaKGoldberg · 2022-11-14T19:59:16Z

@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 · 2023-01-22T18:04:45Z

👋 @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 · 2023-01-24T06:15:59Z

@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 · 2023-01-26T00:38:17Z

...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 · 2023-02-06T20:52:52Z

@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.

chbdetta added 7 commits June 29, 2022 22:49

replicate eslint rule and add base test case

b3b6319

port the eslint lines-around-comment

62efad9

check for comment in interface and allow options

c2433b4

check for comment in type literals and allow options

d97237e

add lines-around-comment in index

e9df0ec

add lines-around-comment in all config

14db2f7

add lines-around-comment doc

7f2661f

JoshuaKGoldberg added the formatting Related to whitespace/bracket formatting. We strongly recommend you use a formatter instead. label Jul 29, 2022

JoshuaKGoldberg mentioned this pull request Aug 7, 2022

Repo: Rule [options] parameter should be non-nullable if defaultOptions exists #5439

Open

JoshuaKGoldberg requested changes Aug 7, 2022

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Aug 7, 2022

prescience-data added a commit to nodesuite/nodesuite that referenced this pull request Sep 23, 2022

feat: disable lines-around-comment for type definitions

8fef1c4

- waiting on typescript-eslint/typescript-eslint#5327

chbdetta added 7 commits September 25, 2022 01:21

delegate non-TS check to the base rule

30825c6

Merge branch 'main' into main

b153dba

fix lint errors

793d2e7

fix doc lint errors

3b74231

address comments

b367c61

address jsdoc comments

b763e3a

proper comment language

59541ed

bradzacher requested changes Oct 4, 2022

View reviewed changes

packages/eslint-plugin/src/rules/lines-around-comment.ts Outdated Show resolved Hide resolved

bradzacher changed the title ~~feat(eslint-plugin): lines-around-comment~~ feat(eslint-plugin): [lines-around-comment] add extension rule Oct 4, 2022

Update packages/eslint-plugin/src/rules/lines-around-comment.ts

f92776d

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Nov 14, 2022

JoshuaKGoldberg mentioned this pull request Nov 14, 2022

Docs: Maintenance > Issue Management > pruning old issues & PRs #5942

Closed

2 tasks

chbdetta requested review from JoshuaKGoldberg and bradzacher and removed request for JoshuaKGoldberg and bradzacher January 24, 2023 06:06

chbdetta and others added 3 commits January 24, 2023 00:33

remove ESilnt base rule test casese

b6b475a

improve coverage

f32e927

Merge branch 'main' into main

75bf6ea

chbdetta requested review from bradzacher and JoshuaKGoldberg and removed request for JoshuaKGoldberg and bradzacher January 25, 2023 20:15

JoshuaKGoldberg requested a review from bradzacher February 6, 2023 22:55

JoshuaKGoldberg approved these changes Feb 19, 2023

View reviewed changes

JoshuaKGoldberg added 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge and removed awaiting response Issues waiting for a reply from the OP or another party labels Feb 19, 2023

bradzacher enabled auto-merge March 13, 2023 02:21

bradzacher added the enhancement: new base rule extension New base rule extension required to handle a TS specific case label Mar 13, 2023

bradzacher approved these changes Mar 13, 2023

View reviewed changes

bradzacher added this pull request to the merge queue Mar 13, 2023

JoshuaKGoldberg removed this pull request from the merge queue due to a manual request Mar 13, 2023

JoshuaKGoldberg merged commit d55211c into typescript-eslint:main Mar 13, 2023

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chbdetta commented Jul 9, 2022 •

edited

Loading

nx-cloud bot commented Jul 9, 2022 •

edited

Loading

typescript-eslint bot commented Jul 9, 2022

netlify bot commented Jul 9, 2022 •

edited

Loading

codecov bot commented Jul 9, 2022 •

edited

Loading

pkuczynski commented Jul 28, 2022

bradzacher commented Jul 28, 2022

pkuczynski commented Jul 29, 2022

JoshuaKGoldberg left a comment

JoshuaKGoldberg Aug 7, 2022

chbdetta Sep 25, 2022 •

edited

Loading

prescience-data commented Sep 23, 2022

bradzacher left a comment

JoshuaKGoldberg commented Nov 14, 2022

JoshuaKGoldberg commented Jan 22, 2023

chbdetta commented Jan 24, 2023

JoshuaKGoldberg commented Jan 26, 2023

chbdetta commented Feb 6, 2023

JoshuaKGoldberg left a comment •

edited

Loading

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

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

Conversation

chbdetta commented Jul 9, 2022 • edited Loading

PR Checklist

Overview

Options

nx-cloud bot commented Jul 9, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented Jul 9, 2022

netlify bot commented Jul 9, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

codecov bot commented Jul 9, 2022 • edited Loading

Codecov Report

pkuczynski commented Jul 28, 2022

bradzacher commented Jul 28, 2022

pkuczynski commented Jul 29, 2022

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Aug 7, 2022

Choose a reason for hiding this comment

chbdetta Sep 25, 2022 • edited Loading

Choose a reason for hiding this comment

prescience-data commented Sep 23, 2022

bradzacher left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Nov 14, 2022

JoshuaKGoldberg commented Jan 22, 2023

chbdetta commented Jan 24, 2023

JoshuaKGoldberg commented Jan 26, 2023

chbdetta commented Feb 6, 2023

JoshuaKGoldberg left a comment • edited Loading

Choose a reason for hiding this comment

chbdetta commented Jul 9, 2022 •

edited

Loading

nx-cloud bot commented Jul 9, 2022 •

edited

Loading

netlify bot commented Jul 9, 2022 •

edited

Loading

codecov bot commented Jul 9, 2022 •

edited

Loading

chbdetta Sep 25, 2022 •

edited

Loading

JoshuaKGoldberg left a comment •

edited

Loading