Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rules): add no-jasmine-globals #116

Merged
merged 4 commits into from May 27, 2018
Merged

feat(rules): add no-jasmine-globals #116

merged 4 commits into from May 27, 2018

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 27, 2018

In preparation for jest-circus, warning users about Jasmine usage would be great.

Will probably add to recommended in next major (whenever eslint 5 comes out)

/cc @captbaritone

@aaronabramov
Copy link
Contributor

oh. this is so awesome!

@mattphillips
Copy link
Member

Looks like coverage has dropped slightly and is breaking CI. Maybe we should add coveralls to this project?

@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

Made jasmine.DEFAULT_TIMEOUT and known asymmetric matchers autofixable. not sure we can do any other ones

@captbaritone
Copy link
Contributor

LGTM. 👏

@mattphillips
Copy link
Member

mattphillips commented May 27, 2018

@SimenB can we add a link to the docs for the recommended approach for the patterns we cannot auto fix?

Edit: or put an example in the error message?

@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

@mattphillips good idea! Feel free to push some examples to the docs 🙂

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looks good. Wonder if we should use stricter language, something stronger than "Avoid using"?

@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

Changed "avoid using" to "illegal usage of"

@thymikee
Copy link
Member

Like it 👍

@thymikee
Copy link
Member

:shipit: when green :)

@SimenB SimenB merged commit 7707e14 into master May 27, 2018
@SimenB SimenB deleted the node-jasmine branch May 27, 2018 18:21
@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

@mattphillips PR welcome with more concrete examples for the docs 🙂

@mattphillips
Copy link
Member

👍

@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

🎉 This PR is included in version 21.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@captbaritone
Copy link
Contributor

@aaronabramov We should turn this on for WWW once you are done.

@captbaritone
Copy link
Contributor

captbaritone commented May 27, 2018

Should this get added to the eslint-plugin-jest readme? If so, I can open a PR.

@thymikee
Copy link
Member

thymikee commented May 27, 2018

Of course! Maybe we can even make it recommended?

@captbaritone
Copy link
Contributor

I'll open a PR.

@captbaritone
Copy link
Contributor

I'll wait on making it recommended since @SimenB said:

Will probably add to recommended in next major (whenever eslint 5 comes out)

@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

Found a bug btw, when applying it to the jest code base. Pushed fix: 885ce17

@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

🎉 This PR is included in version 21.16.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

Interesting that the bot picked up I referenced the commit in this PR, so posted a comment. Probably a bug :P

@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

For posterity, these are the errors reported on the Jest code base with this rule active:

/Users/simen/Development/jest/packages/jest-jasmine2/src/index.js
44:15  error  Illegal usage of jasmine global  jest/no-jasmine-globals

/Users/simen/Development/jest/packages/jest-jasmine2/src/jasmine_async.js
136:15  error  Illegal usage of jasmine global  jest/no-jasmine-globals

/Users/simen/Development/jest/packages/jest-jasmine2/src/jest_expect.js
36:3  error  Illegal usage of jasmine global  jest/no-jasmine-globals
37:3  error  Illegal usage of jasmine global  jest/no-jasmine-globals
38:3  error  Illegal usage of jasmine global  jest/no-jasmine-globals
39:3  error  Illegal usage of jasmine global  jest/no-jasmine-globals
40:3  error  Illegal usage of jasmine global  jest/no-jasmine-globals
42:3  error  Illegal usage of jasmine global  jest/no-jasmine-globals

/Users/simen/Development/jest/testSetupFile.js
16:3  error  Illegal usage of jasmine global  jest/no-jasmine-globals

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.

None yet

5 participants