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): [no-unsafe-enum-comparison] handles switch cases#7541

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

Closed
Zamiell wants to merge12 commits intotypescript-eslint:mainfromIsaacScript:enum-comparison-switch
Closed

feat(eslint-plugin): [no-unsafe-enum-comparison] handles switch cases#7541

Zamiell wants to merge12 commits intotypescript-eslint:mainfromIsaacScript:enum-comparison-switch

Conversation

Zamiell
Copy link
Contributor

@ZamiellZamiell commentedAug 26, 2023•
edited
Loading

PR Checklist

Overview

Hello, co-author of theno-unsafe-enum-comparison rule here.

Currently, heno-unsafe-enum-comparison rule handles comparisons (e.g.BinaryExpression) but it does not handle switch statements. My PR makes the rule handle both.

This was an oversight in my original design of this rule, and I consider this to be a bug. However, I have marked the PR asfeat instead offix to be more conservative; feel free to change it if you wish.

Code Change Summary

I refactored the logic in theBinaryExpression selector and put it inside of a function calledisMismatchedComparison.
Then, I can call that function from multiple selectors. The changes should be pretty straightforward to understand.

Other Notes

I have reworked the documentation for this rule a bit, as the original text is now no longer accurate, because TypeScript has made number enums more safe in version 5.0. Thus, I rewrote the document to focus on the string case, adding a line to emphasize that the rule is still useful if you choose to use string enums over number enums.

auvred reacted with hooray emoji
@typescript-eslint
Copy link
Contributor

Thanks for the PR,@Zamiell!

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.

@netlify
Copy link

netlifybot commentedAug 26, 2023•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commitec4c2e0
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/650878801602780008538d5e
😎 Deploy Previewhttps://deploy-preview-7541--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: 99 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (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-cloud
Copy link

nx-cloudbot commentedAug 26, 2023•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitec4c2e0. 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 20 targets

Sent with 💌 fromNxCloud.

@Zamiell
Copy link
ContributorAuthor

Zamiell commentedAug 26, 2023•
edited
Loading

Oh, interestingly enough, the new version of the rule found a bug atpackages/eslint-plugin/src/rules/no-unnecessary-type-constraint.ts.
I've also taken care of that in this PR so that CI passes.

(Well, CI passes except forcodecov/patch, not sure what to do about that one.)

@bradzacherbradzacher added the enhancement: plugin rule optionNew rule option for an existing eslint-plugin rule labelSep 8, 2023
@bradzacherbradzacher changed the titlefeat: no-unsafe-enum-comparison handles switch casesfeat(eslint-plugin): [no-unsafe-enum-comparison] handles switch casesSep 8, 2023
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.

Great stuff, thanks for such a thorough look & overhaul! 🔥

A few requests for changes, and some nitpicks I'd like to hear your thoughts on.

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelSep 14, 2023
@github-actionsgithub-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party labelSep 18, 2023
@Zamiell
Copy link
ContributorAuthor

Zamiell commentedSep 18, 2023•
edited
Loading

theres some bogus stuff going on in CI, i think a maintainer needs tolook at this, as it looks unrelated to my PR.

JoshuaKGoldberg reacted with thumbs up emoji

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.

Oop, I'd meant to merge changes frommain after merging#7691 but I don't think I have permissions to push to your branch@Zamiell. Pushed to here instead:https://github.com/JoshuaKGoldberg/typescript-eslint/tree/enum-comparison-switch-merge

Ithink the lint failure inErrorsViewer.tsx on theswitch (severity) is an existing bug in the rule that just so happens to be exposed by this PR. Something imported withimport type might not be a value that can actually be runtime-imported. If this is an existing bug, I think it'd be reasonable toeslint-disable it in this PR and file a followup issue. WDYT?

Otherwise 👍 LGTM for merging! We'd want to file a followup issue to get#7691'ssuggest: ... in switch statements too.

@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelOct 19, 2023
@Zamiell
Copy link
ContributorAuthor

how do i give you permission? otherwise, just merge whatever changes from your own branch, as it doesn't matter to me

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commentedOct 22, 2023•
edited
Loading

It's on the bottom-right of the page, at the bottom of the right sidebar:

Screenshot of a checked 'Allowe edits and access to secrets by maintainers' on GitHub

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork is the closest docs page I could find on docs.github.com.

I would prefer to get the changes in this PR so that it's easier to find from the Git history. And you deserve direct/primary author attribution as you've put a lot of great work into this PR! 😄

@Zamiell
Copy link
ContributorAuthor

@JoshuaKGoldberg That button doesn't show up for me. Can you just use the changes from your own branch instead?

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commentedNov 11, 2023•
edited
Loading

Sure, no worries:#7898. I'm guessing you're not seeing the permissions because this PR was sent from an organization (IsaacScript). Either you don't have permissions to enable maintainer merging on that org or that's not doable on GitHub altogether. I don't recall if that's the case.

@Zamiell
Copy link
ContributorAuthor

ok, should i close this PR now?

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commentedNov 13, 2023•
edited
Loading

👍 yup! Just merged#7898.

Thanks again as always@Zamiell!

@ZamiellZamiell deleted the enum-comparison-switch branchNovember 13, 2023 16:03
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsNov 21, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg requested changes

Assignees
No one assigned
Labels
awaiting responseIssues waiting for a reply from the OP or another partyenhancement: plugin rule optionNew rule option for an existing eslint-plugin rule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[no-unsafe-enum-comparison] consider evaluatingswitch statements
3 participants
@Zamiell@JoshuaKGoldberg@bradzacher
[8]ページ先頭
©2009-2025 Movatter.jp