Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
feat(eslint-plugin): [consistent-type-assertions] addarrayLiteralTypeAssertions option#6749
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
feat(eslint-plugin): [consistent-type-assertions] addarrayLiteralTypeAssertions option#6749
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@danvk! 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. |
netlifybot commentedMar 23, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site settings. |
arrayLiteralTypeAssertions option toconsistent-type-assertions rulearrayLiteralTypeAssertions option toconsistent-type-assertions rulearrayLiteralTypeAssertions optionnx-cloudbot commentedMar 23, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@## main #6749 +/- ##==========================================+ Coverage 87.24% 87.25% +0.01%========================================== Files 383 383 Lines 13080 13091 +11 Branches 3845 3849 +4 ==========================================+ Hits 11411 11422 +11 Misses 1302 1302 Partials 367 367
Flags with carried forward coverage won't be shown.Click here to find out more.
|
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.
Sorry this took so long to get to@danvk - the implementation looks very clean and the tests are thorough. Thanks for sending it in!
Just requesting changes on the missing"allow-as-parameter" equivalent.
| type OptUnion = | ||
| | { | ||
| assertionStyle: 'as' | 'angle-bracket'; | ||
| objectLiteralTypeAssertions?: 'allow' | 'allow-as-parameter' | 'never'; | ||
| arrayLiteralTypeAssertions?: 'allow' | 'never'; |
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.
I didn't include an equivalent of the
"allow-as-parameter"option but I can do so if there's a need.
Yeah it feels weird to have one without the other... Could you please?
You most certainly may 😄 I agree with your points. It does seem odd to be so specific, especially since it means the exception doesn't cover |
👋 Hey@danvk! Just checking in, is this still something you have time for? No worries if not - I just don't want to leave it hanging. |
Closing this PR as it's been stale for a few months without activity. Feel free to reopen@danvk if you have time - but no worries if not! If anybody wants to drive it forward, please do post your own PR - and if you use this as a start, consider adding |
Uh oh!
There was an error while loading.Please reload this page.
PR Checklist
Overview
This PR adds an
arrayLiteralTypeAssertionsoption to theconsistent-type-assertionsrule as described in the linked issue. This uses the same auto-fixer as theobjectLiteralTypeAssertionsoption.I didn't include an equivalent of the
"allow-as-parameter"option but I can do so if there's a need.If I may step on my soapbox for a moment:
objectLiteralTypeAssertionsoption (and this new one) are an awkward fit for theconsistent-type-assertions, which you would expect to just enforce consistency betweenx as Tand<T>x. Both options would make more sense in a distinct rule that bans type assertions (rather than enforcing a consistent style).objectLiteralTypeAssertionsbut notarrayLiteralTypeAssertions, or vice versa. It also feels odd that we're singling out these two types but allowing constructs with other types such asconst x = 'foo' as string;