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): [prefer-optional-chain] supportif statement as part of chain#10137

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

Open
omril1 wants to merge51 commits intotypescript-eslint:main
base:main
Choose a base branch
Loading
fromomril1:feat-eslint-plugin-issue-6309-prefer-optional-chain-support-if-statement

Conversation

omril1
Copy link
Contributor

@omril1omril1 commentedOct 12, 2024•
edited
Loading

PR Checklist

Overview

Add anallowIfStatements option that would look atif statements with a single call expression in their body and try to report a suggestion for a bigger chain that includes theif'stest node andbody node

atlowChemi reacted with thumbs up emoji
@typescript-eslint
Copy link
Contributor

Thanks for the PR,@omril1!

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.

@netlifyNetlify
Copy link

netlifybot commentedOct 12, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitcbfb329
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/67d68d4f9892e40008f3ff05
😎 Deploy Previewhttps://deploy-preview-10137--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		1 paths audited
Performance: 94 (🟢 up 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

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

@nx-cloudNx Cloud
Copy link

nx-cloudbot commentedOct 12, 2024•
edited
Loading

View yourCI Pipeline Execution ↗ for commitcbfb329.

CommandStatusDurationResult
nx test eslint-plugin✅ Succeeded7m 44sView ↗
nx run eslint-plugin:test -- --coverage✅ Succeeded6m 14sView ↗
nx test eslint-plugin --coverage=false✅ Succeeded6m 28sView ↗
nx test typescript-estree✅ Succeeded1m 17sView ↗
nx test scope-manager✅ Succeeded<1sView ↗
nx run types:build✅ Succeeded<1sView ↗
nx test typescript-eslint✅ Succeeded10sView ↗
nx run-many --target=build --exclude website --...✅ Succeeded21sView ↗
Additional runs (25)✅ Succeeded...View ↗

☁️Nx Cloud last updated this comment at2025-03-16 08:49:30 UTC

@omril1omril1force-pushed thefeat-eslint-plugin-issue-6309-prefer-optional-chain-support-if-statement branch from41e9ec4 to075c034CompareOctober 12, 2024 11:11
@omril1omril1force-pushed thefeat-eslint-plugin-issue-6309-prefer-optional-chain-support-if-statement branch from075c034 tofd1c08dCompareOctober 12, 2024 11:12
@codecovCodecov
Copy link

codecovbot commentedOct 12, 2024•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.63%. Comparing base(543d36c) to head(cbfb329).
Report is 53 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@##             main   #10137      +/-   ##==========================================+ Coverage   87.58%   87.63%   +0.05%==========================================  Files         470      470                Lines       16095    16162      +67       Branches     4668     4688      +20     ==========================================+ Hits        14097    14164      +67  Misses       1642     1642                Partials      356      356
FlagCoverage Δ
unittest87.63% <100.00%> (+0.05%)⬆️

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

Files with missing linesCoverage Δ
.../rules/prefer-optional-chain-utils/analyzeChain.ts100.00% <100.00%> (ø)
...s/eslint-plugin/src/rules/prefer-optional-chain.ts100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@omril1omril1 marked this pull request as ready for reviewOctober 13, 2024 10:14
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.

Cool! This is a tricky rule to work with and I like how you stayed with the existing flow with it. Nicely done! 👏

I found a few cases that'll need to have support, such as comments in the rule.

Family Guy's Stewie Griffin telling Brian "good stuff. good stuff."

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelNov 4, 2024
@github-actionsgithub-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party labelJan 18, 2025
@omril1
Copy link
ContributorAuthor

@JoshuaKGoldberg ping, I saw a lot of your other work in the typescript/eslint/typescript-eslint repos, and I get that you are busy, but can we please advance this?

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.

Sorry for the delay! I've personally mostly stepped back from reviewing specific rule PRs, especially tricky ones like this. But the change is exciting and I want to be helpful - please do ping if I take weeks to get back to you again.

I think the autofixing/suggesting is still a little too brittle and can be reworked to better handle some comments. Left a few comments suggesting strategies.

? sourceCode.getCommentsBefore(tokenAfterAfterNodeTest)
: []; // if (foo) /* this */

const commentsToReloacte = [

Choose a reason for hiding this comment

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

[Typo]

Suggested change
constcommentsToReloacte=[
constcommentsToRelocate=[

omril1 reacted with thumbs up emoji
@JoshuaKGoldberg
Copy link
Member

👋 Just checking in@omril1, is this still something you have time for?

@JoshuaKGoldbergJoshuaKGoldberg added the stalePRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period labelApr 14, 2025
@omril1
Copy link
ContributorAuthor

👋 Just checking in@omril1, is this still something you have time for?

@JoshuaKGoldberg I think I'm giving up on this. The rule as it is currently doesn't handle all of the comments between the optional chaining nodes, and this PR is reusing all of the existing logic fromanalyzeChain.

I have no interest in expanding the scope of the PR, as it was already hard enough for me to integrateIfStatement into the current logic.

@JoshuaKGoldberg
Copy link
Member

Ah, I'm sorry to hear that - especially since I think this is largely my fault for trying to sling a lot of annoying comments work on you. I think what was missing on my end was I'd thought there was existing comment handling that was being regressed here. But I think it'd be better to get this in and file a followup issue for the comments. Do you think you'd have time to just wontfix my comments there?

@omril1
Copy link
ContributorAuthor

Ah, I'm sorry to hear that - especially since I think this is largely my fault for trying to sling a lot of annoying comments work on you. I think what was missing on my end was I'd thought there was existing comment handling that was being regressed here. But I think it'd be better to get this in and file a followup issue for the comments. Do you think you'd have time to just wontfix my comments there?

I'm not sure what 'wontfix' mean, do you want me to just acknowledge it as out of scope in each discussion?

I'll probably try to pick easier PRs to work on next time, I just liked how this rule helped simplify the awful legacy codebases I worked on 😄

@JoshuaKGoldberg
Copy link
Member

Exactly: whatever you don't have scope to do, mention that in the conversation.

And yeah, this is one of my favorite rules too. Really a pity what a nightmare working with its implementation is. 🥲

@JoshuaKGoldbergJoshuaKGoldberg added triageWaiting for team members to take a look and removed awaiting responseIssues waiting for a reply from the OP or another party stalePRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period labelsApr 15, 2025
@JoshuaKGoldbergJoshuaKGoldberg self-requested a reviewApril 15, 2025 13:13
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@kirkwaiblingerkirkwaiblingerkirkwaiblinger left review comments

@JoshuaKGoldbergJoshuaKGoldbergAwaiting requested review from JoshuaKGoldberg

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
triageWaiting for team members to take a look
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@omril1@JoshuaKGoldberg@kirkwaiblinger
[8]ページ先頭
©2009-2025 Movatter.jp