Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

Zamiell
Copy link
Contributor

@Zamiell Zamiell commented Aug 26, 2023

PR Checklist

Overview

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

Currently, he no-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 as feat instead of fix to be more conservative; feel free to change it if you wish.

Code Change Summary

I refactored the logic in the BinaryExpression selector and put it inside of a function called isMismatchedComparison.
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.

@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 on https://opencollective.com/typescript-eslint.

@netlify
Copy link

netlify bot commented Aug 26, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit ec4c2e0
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/650878801602780008538d5e
😎 Deploy Preview https://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 your Netlify site configuration.

@Zamiell
Copy link
Contributor Author

Zamiell commented Aug 26, 2023

Oh, interestingly enough, the new version of the rule found a bug at packages/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 for codecov/patch, not sure what to do about that one.)

@bradzacher bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label Sep 8, 2023
@bradzacher bradzacher changed the title feat: no-unsafe-enum-comparison handles switch cases feat(eslint-plugin): [no-unsafe-enum-comparison] handles switch cases Sep 8, 2023
Copy link
Member

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

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Sep 14, 2023
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Sep 18, 2023
@Zamiell
Copy link
Contributor Author

Zamiell commented Sep 18, 2023

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

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

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 from main 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

I think the lint failure in ErrorsViewer.tsx on the switch (severity) is an existing bug in the rule that just so happens to be exposed by this PR. Something imported with import type might not be a value that can actually be runtime-imported. If this is an existing bug, I think it'd be reasonable to eslint-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's suggest: ... in switch statements too.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Oct 19, 2023
@Zamiell
Copy link
Contributor Author

Zamiell commented Oct 20, 2023

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 commented Oct 22, 2023

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
Contributor Author

Zamiell commented Nov 9, 2023

@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 commented Nov 11, 2023

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
Contributor Author

Zamiell commented Nov 12, 2023

ok, should i close this PR now?

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Nov 13, 2023

👍 yup! Just merged #7898.

Thanks again as always @Zamiell!

@Zamiell Zamiell deleted the enum-comparison-switch branch November 13, 2023 16:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party enhancement: plugin rule option New rule option for an existing eslint-plugin rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[no-unsafe-enum-comparison] consider evaluating switch statements
3 participants