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-unsafe-assignment] be more specific about error types #8304

Merged

Conversation

tobySolutions
Copy link
Contributor

PR Checklist

Overview

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @tobySolutions!

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 26, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit b70a2ca
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/665d2fb149db430008917a53
😎 Deploy Preview https://deploy-preview-8304--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: 90 (🔴 down 8 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.

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.

A great start, thanks again for working on this with me!

Requesting changes mostly around adding in more testing. A couple small refactors too. But the functionality looks great! ✨

packages/eslint-plugin/src/rules/no-unsafe-assignment.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-unsafe-assignment.ts Outdated Show resolved Hide resolved
packages/type-utils/src/predicates.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jan 29, 2024
@JoshuaKGoldberg
Copy link
Member

👋 @tobySolutions ping, just checking in - is this something you still have the time and energy for? It's still in draft & there are some CI ❌s so we haven't looked at it.

@tobySolutions
Copy link
Contributor Author

👋 @tobySolutions ping, just checking in - is this something you still have the time and energy for? It's still in draft & there are some CI ❌s so we haven't looked at it.

Definitely Josh! I will resume work on this in a bit. Thank you so very much for checking in! 🙌

@JoshuaKGoldberg JoshuaKGoldberg added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Jun 2, 2024
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review June 2, 2024 19:32
@JoshuaKGoldberg JoshuaKGoldberg removed awaiting response Issues waiting for a reply from the OP or another party stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period labels Jun 2, 2024
@JoshuaKGoldberg JoshuaKGoldberg self-requested a review June 2, 2024 19:32
yarn.lock Outdated Show resolved Hide resolved
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.

Progress! 🚀

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jun 2, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title fix: error assignment type issue fix(eslint-plugin): [no-unsafe-assignment] be more specific about error types Jun 2, 2024
JoshuaKGoldberg
JoshuaKGoldberg previously approved these changes Jun 3, 2024
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! Thanks for working with me on this @tobySolutions. I'm excited to see a better error message for no-unsafe-assignment on production soon!

Only one change request left from me, to add a bit more to the tests. It's a pretty small thing - so marking as 1 approval to let someone else from the team chime in if the want.

Cartoon cat happily jumping for joy. Caption: "YAY!!!"

@JoshuaKGoldberg JoshuaKGoldberg added 1 approval One team member has approved this PR; a second should be enough to merge it and removed awaiting response Issues waiting for a reply from the OP or another party labels Jun 3, 2024
@tobySolutions
Copy link
Contributor Author

🙌 Great! Thanks for working with me on this @tobySolutions. I'm excited to see a better error message for no-unsafe-assignment on production soon!

Only one change request left from me, to add a bit more to the tests. It's a pretty small thing - so marking as 1 approval to let someone else from the team chime in if the want.

Cartoon cat happily jumping for joy. Caption: "YAY!!!" Cartoon cat happily jumping for joy. Caption: "YAY!!!"

Yay!!!

@Josh-Cena
Copy link
Member

If it only changes one rule, then it doesn't fix #8231, only partially

@JoshuaKGoldberg
Copy link
Member

Yup - I'm about to post an edit splitting the issue up 😄

@Josh-Cena
Copy link
Member

What about other no-unsafe rules?

@JoshuaKGoldberg
Copy link
Member

🙌 Filed!

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.

This is wonderful, thanks @tobySolutions!

We've got a bunch of issues filed to apply similar improvements to other no-unsafe-* rules. I think once we've gone through those we'll be well-positioned to review our messaging around "error" types and maybe iterate further on how they're explained to users. But that's a battle for another day.

Nicholas Cage happily smiling with long hair flowing in a breeze. His head fades into a cosmic galaxy kind of fly-through

@JoshuaKGoldberg JoshuaKGoldberg merged commit 293fb24 into typescript-eslint:main Jun 5, 2024
59 of 60 checks passed
@tobySolutions
Copy link
Contributor Author

This is wonderful, thanks @tobySolutions!

We've got a bunch of issues filed to apply similar improvements to other no-unsafe-* rules. I think once we've gone through those we'll be well-positioned to review our messaging around "error" types and maybe iterate further on how they're explained to users. But that's a battle for another day.

Nicholas Cage happily smiling with long hair flowing in a breeze. His head fades into a cosmic galaxy kind of fly-through Nicholas Cage happily smiling with long hair flowing in a breeze. His head fades into a cosmic galaxy kind of fly-through

Thank you so much for being super awesome Josh!!! This was such an amazing experience. I look forward to more contributions

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Jul 16, 2024

We'll also need a no-duplicate-type-constitutents version of this I think, that doesn't just display the name differently, but also impacts the fixer logic (see also #9344 (comment))

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval One team member has approved this PR; a second should be enough to merge it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: [no-unsafe-assignment] Differentiate a types-error any from a "true" any
4 participants