feat(rules): option for rule to allow uppercase for top level describes #428
Conversation
There was a problem hiding this comment.
Thanks for this addition!
For now, I've just left the one change request, as I think that'll let you simplify a lot of the logic.
I could be missing something, but in my head the implementation I'm imaging would simply require an early return after the isJestFunctionWithLiteralArg condition, based on the boolean option & a call to isTopMostFunction.
Happy to discuss if you've got questions 🙂
| @@ -93,7 +120,7 @@ export default createRule({ | |||
| properties: { | |||
| ignore: { | |||
| type: 'array', | |||
| items: { enum: ['describe', 'test', 'it'] }, | |||
| items: { enum: ['describe', 'test', 'it', 'top'] }, | |||
There was a problem hiding this comment.
I think if you're going to do it this way, it would be better to use a boolean option since top isn't a jest keyword and so doesn't really make sense having it implemented in the same manner of isTestCase & isDescribe, nor in the JestFunctionName union.
That would also be a good position to later extend it to be an array accepting the same properties as ignore.
There was a problem hiding this comment.
My understanding is that if I gave ignore=top, the lowercase rule will not apply to the first parent element(describe/test/it) the inner describe/test should have this rule.
If I do an early return as you suggested the inner elements will also be skipped off the rule. Which is
describe("Xyz abc"){
test("Cde"){}
}
will be a valid one
There was a problem hiding this comment.
Just to be clear, this is the sort of early return I had in mind:
if(ignoreTopLevel && isTopMostJestFunction(node)) {
return;
}
Since CallExpression is called for every CallExpression that ESLint finds, the early return won't interfere with the inner jest function calls.
src/rules/lowercase-name.ts
Outdated
|
|
||
| const topMethod = jestTopFunctioName(node, counter); | ||
|
|
||
| const topMethodIgnore = topMethod && ignore.includes(TopCase.top); |
There was a problem hiding this comment.
The only value being used in this conditional that is computed as part of the CallExpression function is topMethod, meaning you could move ignore.includes(TopCase.top) out of this function, and assign it to a variable like you do w/ counter.
That way we won't be iterating over ignores for every CallExpression that makes it this far - a micro optimisation for sure, but one that's zero-effort, so might as well.
|
@G-Rath made changes based on your comments. Can you review it again and help me out |
| @@ -37,7 +37,8 @@ | |||
| "typecheck": "tsc -p ." | |||
| }, | |||
| "dependencies": { | |||
| "@typescript-eslint/experimental-utils": "^1.13.0" | |||
| "@typescript-eslint/experimental-utils": "^1.13.0", | |||
| "build": "^0.1.4" | |||
| @@ -652,6 +660,13 @@ export const isDescribe = ( | |||
| ); | |||
| }; | |||
|
|
|||
| export function isTopMostJestFunction(node: TSESTree.CallExpression): boolean { | |||
There was a problem hiding this comment.
I'd prefer that this be inside the rule, rather than in utils - iirc there is another rule doing something similar, but I'm going to be doing a spring-cleaning of the codebase this weekend, so I can refactor this into a utility method in that PR.
For now, it'll just be cleaner to have it in the rule :)
| defaultOptions: [{ ignore: [], allowedPrefixes: [] }], | ||
| create(context, [{ ignore = [], allowedPrefixes = [] }]) { | ||
| return { | ||
| CallExpression(node) { | ||
| if (!isJestFunctionWithLiteralArg(node)) { | ||
| return; | ||
| } | ||
|
|
||
| const topMethod = jestTopFunctioName(node, isTopMostJestFunction(node)); | ||
| if (topMethod && ignore.includes(TopCase.top)) { |
|
@gayathrigs27 Let me know if you're interested in championing this PR, as it would be cool to get this across the line! I'm happy to pick it up & finish it off if you'd prefer 😄 |
|
🎉 This issue has been resolved in version 23.17.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.