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

fix(eslint-plugin): [no-redundant-type-constituents] incorrectly marks & string as redundant #8282

Merged
merged 6 commits into from Mar 16, 2024

Conversation

arka1002
Copy link
Contributor

fixes: #7580

PR Checklist

Overview

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @arka1002!

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.

Copy link

netlify bot commented Jan 21, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 52783f0
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/65dc66bfdc23b5000856ca1b
😎 Deploy Preview https://deploy-preview-8282--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 (🟢 up 9 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.

@arka1002 arka1002 marked this pull request as draft January 21, 2024 12:50
@arka1002 arka1002 marked this pull request as ready for review January 28, 2024 10:53
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.

Ok! This rule is trickier than I remembered it being 😅 nice job adding to its logic. Thanks for sending this!

I think I understand what the new code is doing, but it was a little hard to read through. A couple general suggestions:

  • Try to stick with the existing conventions in code unless there's a strong reason to deviate
  • Try to use informative names when possible, not general ones like key or iterator

The core logic seems reasonable to me. Requesting changes on refactors. There might be a point where more tests would be reasonable depending on one or two of them?

Comment on lines 341 to 343
value.forEach(union => {
typeFlagsOfUnions.push(union.typeFlags);
});
Copy link
Member

Choose a reason for hiding this comment

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

[Style] Is there a need to have a shared variable outside the forEach? Switching it to being inside the .forEach and never modified doesn't fail any unit tests:

Suggested change
value.forEach(union => {
typeFlagsOfUnions.push(union.typeFlags);
});
const typeFlagsOfUnions = unionTypeFlags.flatMap(
union =>
union.typeFlags as keyof typeof literalToPrimitiveTypeFlags,
);

That includes an extra tip: since we know the type flags are all the keyof typeof literalToPrimitiveTypeFlags, using the assertion early on means we don't need multiple assertions later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the variable, I dont think we need it anymore.
I might need some help with the type assertions though 😅

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 2, 2024
arka1002 and others added 2 commits February 25, 2024 18:04
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Copy link

codecov bot commented Feb 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.07%. Comparing base (025e892) to head (52783f0).
Report is 43 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8282      +/-   ##
==========================================
- Coverage   87.84%   87.07%   -0.77%     
==========================================
  Files         397      251     -146     
  Lines       13844    12285    -1559     
  Branches     4073     3871     -202     
==========================================
- Hits        12161    10697    -1464     
+ Misses       1381     1316      -65     
+ Partials      302      272      -30     
Flag Coverage Δ
unittest 87.07% <100.00%> (-0.77%) ⬇️

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

Files Coverage Δ
...plugin/src/rules/no-redundant-type-constituents.ts 93.91% <100.00%> (+0.78%) ⬆️

... and 146 files with indirect coverage changes

@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 3, 2024
@bradzacher bradzacher changed the title feat(eslint-plugin): [no-redundant-type-constituents] incorrectly marks & string as redundant fix(eslint-plugin): [no-redundant-type-constituents] incorrectly marks & string as redundant Mar 16, 2024
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

@bradzacher bradzacher added the bug Something isn't working label Mar 16, 2024
@bradzacher bradzacher merged commit 093225c into typescript-eslint:main Mar 16, 2024
62 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2024
@arka1002 arka1002 deleted the issue-7580 branch March 24, 2024 14:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: no-redundant-type-constituents incorrectly marks & string as redundant
3 participants