Skip to content

Conversation

@brianlmacdonald
Copy link
Contributor

Closes #90.

Adds the eslint rule 'no-jest-import', disallowing any import from Jest.

Adds tests and documentation for rule.

Copy link
Contributor

@ranyitz ranyitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -0,0 +1,20 @@
# Disallow importing Jest(no-jest-import)

The jest object is automatically in scope within every test file. The methods in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a code block for the jest object.


The jest object is automatically in scope within every test file. The methods in
the jest object help create mocks and let you control Jest's overall behavior.
It is therefore completely unnecessary to import in Jest, as Jest doesn't export
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change Jest to jest here, as you refer to the object.


This rule reports on any importing of Jest.

To name a few: *var jest = require('jest'); *const jest = require('jest');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use code blocks here?

  • var jest = require('jest');
  • const jest = require('jest');
  • import jest from 'jest';
  • import {jest as test} from 'jest';

To name a few: *var jest = require('jest'); *const jest = require('jest');
*import jest from 'jest'; *import {jest as test} from 'jest';

There is no correct usage of this code, other than to not import Jest it in the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest => jest

if (node.source.value === 'jest') {
context.report({
node,
message: `Jest is automatically in scope. Do not import Jest, as it doesn't export anything.`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message can be moved into a variable so you won't write it twice.

index.js Outdated
'jest/no-disabled-tests': 'warn',
'jest/no-focused-tests': 'error',
'jest/no-identical-title': 'error',
'jest/no-jest-import': 'error',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically breaking to add it here, mind removing it?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comments

@SimenB
Copy link
Member

SimenB commented Mar 3, 2018

@ranyitz thanks for reviewing! 😀

@SimenB SimenB force-pushed the feat/nojestimportrule branch from 88413a0 to 52b3494 Compare March 5, 2018 16:32
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants