Add lowercase-description rule #56
Conversation
ismail-syed
force-pushed
the
test-description-lowercase
branch
from
bd752c2
to
f213dd3
Compare
| 'test("foo", function () {})', | ||
| ], | ||
|
|
||
| invalid: [ |
There was a problem hiding this comment.
I think it should warn for describe and test as well
There was a problem hiding this comment.
I agree, I think that'd be nice!
|
@ismail-syed ping 🙂 |
ismail-syed
force-pushed
the
test-description-lowercase
branch
2 times, most recently
from
0d1a166
to
1fe99d6
Compare
|
Sorry for the delay, thanks for the pin @SimenB. |
There was a problem hiding this comment.
I'm not sure why the build is failing on Node 6 or Node 8 (looks like an issue with commitlint), but object destructuring is causing the Node 4 build failure (I commented on the problematic line).
I commented with some other suggestions and edge cases that I saw as well.
rules/lowercase_description.js
Outdated
| }; | ||
|
|
||
| const testDescription = node => { | ||
| const { type } = node.arguments[0]; |
There was a problem hiding this comment.
Object destructuring is not supported on Node 4, which is causing the Node 4 failure on Travis.
Related: #64 (comment)
rules/lowercase_description.js
Outdated
| }; | ||
|
|
||
| const isItDescription = node => { | ||
| const foo = |
There was a problem hiding this comment.
Minor suggestion: return this conditional statement instead of assigning to foo, since the variable doesn't explain what's going on in this function.
There was a problem hiding this comment.
Ops, forgot to remove this when I was poking around. Nice catch.
rules/lowercase_description.js
Outdated
|
|
||
| const ruleMsg = 'it() description should begin with lowercase'; | ||
|
|
||
| const isItOrTestorDescribeFunction = node => { |
There was a problem hiding this comment.
Minor function renaming suggestion: isItTestOrDescribeFunction
rules/lowercase_description.js
Outdated
| meta: { | ||
| docs: { | ||
| url: | ||
| 'https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/lowercase_description.md', |
There was a problem hiding this comment.
lowercase_description should be lowercase-description to match the lowercase-description.md filename.
I think if you have master merged in, this is autofixable with ESLint (introduced in #60).
There was a problem hiding this comment.
I did have the merged changes. The issue was that the file name was rules/lowercase_description.js, instead of rules/lowercase-description.js. Looks like the rule enforces the expected doc to be present based on the current file name.
rules/lowercase_description.js
Outdated
| const descriptionBeginsWithLowerCase = node => { | ||
| if (isItOrTestorDescribeFunction(node) && isItDescription(node)) { | ||
| const description = testDescription(node); | ||
| return description[0] === description[0].toLowerCase(); |
There was a problem hiding this comment.
Looking at the definition of testDescription(), it can technically return undefined if the first argument's type is not Literal or TemplateLiteral, or if the Literal provided is not a string.
For example, the following code:
it(42, () => {})will result in TypeError: Cannot read property 'toLowerCase' of undefined since the first argument of it() is a Literal, but it's not a string.
While passing a number as the first argument of it() isn't really valid, I think this rule should protect against a possibility like this.
There was a problem hiding this comment.
Good catch, updated and added some tests to check this as well.
ismail-syed
force-pushed
the
test-description-lowercase
branch
from
1fe99d6
to
ea38aa7
Compare
rules/lowercase-description.js
Outdated
| @@ -0,0 +1,64 @@ | |||
| 'use strict'; | |||
|
|
|||
| const ruleMsg = 'it() description should begin with lowercase'; | |||
There was a problem hiding this comment.
the it part of the message should reflect test or describe as well
ismail-syed
force-pushed
the
test-description-lowercase
branch
4 times, most recently
from
7d0f175
to
e87088c
Compare
SimenB
force-pushed
the
test-description-lowercase
branch
from
e87088c
to
45396c1
Compare
Fixes #46