feat: add no-mocks-import rule #246
Conversation
Generated by 🚫 dangerJS |
SimenB
left a comment
SimenB
left a comment
There was a problem hiding this comment.
Hell yeah, this is awesome! Just update the table in the readme, and this should be good to go
jeysal
force-pushed
the
no-mocks-imports
branch
from
9d4b26d to
ca843e3
Compare
|
Updated readme, thanks Danger 😅 |
|
I assume the changelog file doesn't need to be updated? It does not seem very complete 😅 |
|
Nope - it's supposed to be generated, but it messed something up so I disabled it |
SimenB
left a comment
SimenB
left a comment
There was a problem hiding this comment.
Bah, forgot to submit the review
index.js
Outdated
| 'jest/no-focused-tests': 'error', | ||
| 'jest/no-identical-title': 'error', | ||
| 'jest/no-jest-import': 'error', | ||
| 'jest/no-mocks-import': 'error', |
There was a problem hiding this comment.
Is it worth the break?
If yes, we should look at all rules and land a single pr updating the recommended rules
There was a problem hiding this comment.
I think it's always an error so it belongs in recommended? For linting I'd even say just release new default rules when they're ready, it's better to have many small majors than one big major where people see hundreds of errors after upgrading and decide to just disable the new rules because they're annoying ^^
There was a problem hiding this comment.
It's still a breaking change. And it might not always be an error even though I agree it's not a good idea
There was a problem hiding this comment.
So to be clear, are you suggesting not to add it to recommended or to add it to recommended at a later point?
There was a problem hiding this comment.
Add it to recommended at a later point :)
There was a problem hiding this comment.
Okay. We can leave the line adding it to recommended commented out to keep track. Are there any other rules that you'd want to go into recommended in the next major?
There was a problem hiding this comment.
Yeah, a comment is a good idea.
I'd like to activate the ones for using circus (banning jasmine globals) at least
There was a problem hiding this comment.
I changed it. Didn't add a commented out one for jasmine globals because it's already there (just as warn)
BREAKING CHANGE: Add no-mocks-import to recommended rules
jeysal
force-pushed
the
no-mocks-imports
branch
from
ca843e3 to
5fc1c98
Compare
|
The publish failed for some reason... https://travis-ci.org/jest-community/eslint-plugin-jest/jobs/523169312#L857 Manually published 22.5.0 now |
|
Hello, we tried this new rule but it's failing on dynamic requires. Can you please check it? Thanks! :) #249 |
5 participants
@thymikee You can cross this off your todo list ;)
@SimenB requesting review :D
Alternative to using
pathwould have been/(\/|^)__mocks__(\/|$)/, but I found this easier to read.Potential config option would be
allowImportBetweenMocksthat disables the rule inside of mocks (IIRC ESLint can give you the path of the file being linted) so you can compose mocks, but I don't consider that to be a good pattern usually so I think you should eslint-disable if you really want to do that.BREAKING CHANGE:
Add no-mocks-import to recommended rules