-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [no-base-to-string] add checkUnknown Option #11128
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
base: main
Are you sure you want to change the base?
feat(eslint-plugin): [no-base-to-string] add checkUnknown Option #11128
Conversation
Thanks for the PR, @developer-bandi! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
View your CI Pipeline Execution ↗ for commit 3bdc647.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Could you please get a passing CI so we can merge this though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(wrong button)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11128 +/- ##
==========================================
+ Coverage 90.84% 90.92% +0.08%
==========================================
Files 497 499 +2
Lines 50320 50855 +535
Branches 8311 8387 +76
==========================================
+ Hits 45714 46241 +527
- Misses 4591 4599 +8
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
I added an option description to the document because there was an error that occurred not only in the CI error but also in the document due to not adding the option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! 🚀
Just a couple small comment on the meta property, nothing we can't touch up before merge.
type: 'boolean', | ||
default: false, | ||
description: | ||
'Checks the case where toString is applied to unknown type', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Docs] A little bit of phrasing alignment:
'Checks the case where toString is applied to unknown type', | |
'Whether to also check values of type `unknown`', |
@@ -46,6 +47,12 @@ export default createRule<Options, MessageIds>({ | |||
type: 'object', | |||
additionalProperties: false, | |||
properties: { | |||
checkUnknown: { | |||
type: 'boolean', | |||
default: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Cleanup] This actually doesn't do anything in our stack (long story...)
default: false, |
PR Checklist
unknown
type #10862Overview
add checkUnknown option.
One thing I'm curious about is, is it correct to treat generic types as unknown?