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

Add guard to arguments #58

Merged
merged 4 commits into from Dec 3, 2018
Merged

Add guard to arguments #58

merged 4 commits into from Dec 3, 2018

Conversation

tinovyatkin
Copy link
Contributor

I'm getting an error cannot read property [0] of undefined point to arguments access here while using ESLint in VSCode, it seems like ESLint while running on incomplete code can call this with arguments undefined. Adding guard should fix that.

I'm getting an error `cannot read property [0] of undefined` point to `arguments` access here while using ESLint in VSCode, it seems like ESLint while running on incomplete code can call this with `arguments` undefined. Adding guard should fix that.
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.

Can you run eslint --fix and maybe adjust commit message?

@SimenB maybe we shouldn't fail the CI in such case as we can adjust the commit message before merging, or at least run it as a separate job

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.

This needs a test which fails without this change

@SimenB
Copy link
Member

SimenB commented Feb 6, 2018

@thymikee Not sure I follow. commitlint runs separately already, so it just fails the whole build, it doesn't stop the next script from running. We could (and maybe should) separate lint and test though, so one can see test output even if lint fails 🙂

@thymikee
Copy link
Member

thymikee commented Feb 6, 2018

We could (and maybe should) separate lint and test though, so one can see test output even if lint fails 🙂

This is what I meant 👍

@SimenB
Copy link
Member

SimenB commented Feb 6, 2018

911bba1

@SimenB
Copy link
Member

SimenB commented Feb 11, 2018

@tinovyatkin ping 🙂Just adding a code snippet which shows the error you got would be great!

@steelbrain
Copy link

eslint-plugin-jest is crashing for me at expect(undefined).toBe on v22.0.1
I think this PR will fix it

The stack trace of the failure is

TypeError: Cannot read property '0' of undefined
at argument (node_modules/eslint-plugin-jest/rules/util.js:76:54)
at expectToBeCase (node_modules/eslint-plugin-jest/rules/util.js:34:3)
...

@SimenB
Copy link
Member

SimenB commented Dec 3, 2018

Great, thanks @steelbrain! Those indeed reproduced the issue, and the change in this PR fixes it. I added that as an assertion and will merge when CI is happy 🙂

@SimenB SimenB merged commit 73c9187 into jest-community:master Dec 3, 2018
@SimenB
Copy link
Member

SimenB commented Dec 3, 2018

🎉 This PR is included in version 22.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants