Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
JoshuaKGoldberg merged 24 commits intotypescript-eslint:mainfromchbdetta:main
Mar 13, 2023

Conversation

chbdetta
Copy link
Contributor

@chbdettachbdetta commentedJul 9, 2022•
edited
Loading

PR Checklist

Overview

port theeslint/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;

sotnikov-link, pkuczynski, cassus, Samcn21, and armano2 reacted with thumbs up emoji
@nx-cloud
Copy link

nx-cloudbot commentedJul 9, 2022•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit75bf6ea. 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 💌 fromNxCloud.

@typescript-eslint
Copy link
Contributor

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 onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day.

@netlify
Copy link

netlifybot commentedJul 9, 2022•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit75bf6ea
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/63d06dff2fcfa900090ed05e
😎 Deploy Previewhttps://deploy-preview-5327--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@codecov
Copy link

codecovbot commentedJul 9, 2022•
edited
Loading

Codecov Report

Merging#5327 (75bf6ea) intomain (67706e7) willincrease coverage by0.07%.
The diff coverage is100.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
FlagCoverage Δ
unittest91.44% <100.00%> (+0.07%)⬆️

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

Impacted FilesCoverage Δ
...ckages/eslint-plugin/src/util/getESLintCoreRule.ts75.00% <ø> (ø)
...es/eslint-plugin/src/rules/lines-around-comment.ts100.00% <100.00%> (ø)

@pkuczynski
Copy link

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

@bradzacher
Copy link
Member

@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 adocumented 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 a4,000 line PR.

@JoshuaKGoldbergJoshuaKGoldberg added the formattingRelated to whitespace/bracket formatting. We strongly recommend you use a formatter instead. labelJul 29, 2022
@pkuczynski
Copy link

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 reacted with heart emoji

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

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

pkuczynski and chbdetta reacted with heart emoji
/**
* Returns whether or not comments are at the array start or not.
*/
function isCommentAtArrayStart(token: TSESTree.Token): boolean {

Choose a reason for hiding this comment

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

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:

construles=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.

Copy link
ContributorAuthor

@chbdettachbdettaSep 25, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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 theProgram() from the base rule, I have to create a customcontext.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 in30825c6

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelAug 7, 2022
@prescience-data
Copy link

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

prescience-data added a commit to nodesuite/nodesuite that referenced this pull requestSep 23, 2022
Copy link
Member

@bradzacherbradzacher left a comment

Choose a reason for hiding this comment

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

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

@bradzacherbradzacher changed the titlefeat(eslint-plugin): lines-around-commentfeat(eslint-plugin): [lines-around-comment] add extension ruleOct 4, 2022
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
@JoshuaKGoldberg
Copy link
Member

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

viveleroi reacted with thumbs up emoji

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelNov 14, 2022
@JoshuaKGoldberg
Copy link
Member

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

@chbdettachbdetta requested review fromJoshuaKGoldberg andbradzacher and removed request forJoshuaKGoldberg andbradzacherJanuary 24, 2023 06:06
@chbdetta
Copy link
ContributorAuthor

@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.tsERROR:   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]

@chbdettachbdetta requested review frombradzacher andJoshuaKGoldberg and removed request forJoshuaKGoldberg andbradzacherJanuary 25, 2023 20:15
@JoshuaKGoldberg
Copy link
Member

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

@chbdetta
Copy link
ContributorAuthor

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

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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 reacted with heart emoji
@JoshuaKGoldbergJoshuaKGoldberg 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 responseIssues waiting for a reply from the OP or another party labelsFeb 19, 2023
@bradzacherbradzacher added the enhancement: new base rule extensionNew base rule extension required to handle a TS specific case labelMar 13, 2023
@bradzacherbradzacher added this pull request to the merge queueMar 13, 2023
@JoshuaKGoldbergJoshuaKGoldberg removed this pull request from the merge queue due to a manual requestMar 13, 2023
@JoshuaKGoldbergJoshuaKGoldberg merged commitd55211c intotypescript-eslint:mainMar 13, 2023
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsMar 21, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg approved these changes

@bradzacherbradzacherbradzacher approved these changes

Assignees
No one assigned
Labels
1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we mergeenhancement: new base rule extensionNew base rule extension required to handle a TS specific caseformattingRelated to whitespace/bracket formatting. We strongly recommend you use a formatter instead.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[lines-around-comment] Does not respect interfaces
6 participants
@chbdetta@pkuczynski@bradzacher@prescience-data@viveleroi@JoshuaKGoldberg
[8]ページ先頭
©2009-2025 Movatter.jp