Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
chore(eslint-plugin-internal): [plugin-test-formatting] support random object literal tests#5895
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nx-cloudbot commentedOct 27, 2022 • 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.
Thanks for the PR,@bradzacher! 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 commentedOct 27, 2022 • 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. |
Uh oh!
There was an error while loading.Please reload this page.
codecovbot commentedOct 27, 2022 • 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 #5895 +/- ##==========================================- Coverage 91.32% 91.29% -0.03%========================================== Files 365 365 Lines 12212 12251 +39 Branches 3564 3577 +13 ==========================================+ Hits 11152 11185 +33- Misses 752 758 +6 Partials 308 308
Flags with carried forward coverage won't be shown.Click here to find out more.
|
…m object literal tests
74828bc to9d928caCompareThere 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.
Super, thanks! ✨
| import { RuleTester } from '../RuleTester'; | ||
| const ruleTester = new RuleTester({ | ||
| parser: '@typescript-eslint/parser', | ||
| }); | ||
| const grouped:TSESLint.RunTests<MessageIds, Options> = { | ||
| const grouped: RunTests<MessageIds, Options> = { |
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.
Why don't we just pass this directly toruleTester.run? There's only one variable here.
| import { RuleTester } from '../../RuleTester'; | ||
| const ruleTester = new RuleTester({ | ||
| parser: '@typescript-eslint/parser', | ||
| }); | ||
| const sortedCiWithoutGrouping:TSESLint.RunTests<MessageIds, Options> = { | ||
| const sortedCiWithoutGrouping: RunTests<MessageIds, Options> = { |
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.
Similar suggestion: I like the idea behind describing the variables, but it just seems like extra cruft to me. How about passing them directly toruleTester.run?
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'm personally not too miffed either way.
I think the separate variables is nice as a way to document and group the tests.
it's much clearer than the alternative:
ruleTester.run('member-ordering',rule,{valid:[//// Sorted case insensitive without grouping//'test1',// ...//// Sorted case insensitive with grouping//'testn+1',// ...],});
I find it really easy to lose the comments in the array when it gets really big.
But the indirection is a bit of a pain overall.
I think either way has trade-offs, so IDK which one we want as the standard in the repo.
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.
My mild preference would be for less code, but it's not a hill I'm going to die on 😄
| testProp.computed || | ||
| testProp.key.type !== AST_NODE_TYPES.Identifier || | ||
| testProp.key.name !== 'errors' || | ||
| testProp.value.type !== AST_NODE_TYPES.ArrayExpression |
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.
Nit: this isalmost the same as the previous.type === AST_NODE_TYPES.SpreadElement || ... check, but for the'errors' case. I wonder if there's a good way to dedup? 🤷 definitely not a blocker IMO.
# Conflicts:#packages/eslint-plugin-internal/package.json
Overview
I was looking at some of our tests and noticed they weren't formatted correctly.
Investigating it was because they were declared as variables instead of as part of a test run.
This PR just adds support to the rule for random object literals by doing some fuzzy checks using types.