-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
fix(expect-single-argument): fix error when providing no arguments an… #375
fix(expect-single-argument): fix error when providing no arguments an… #375
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick reply.
lib/rules/expect-single-argument.js
Outdated
@@ -14,7 +14,7 @@ function create (context) { | |||
message: 'Expect must have a single argument. More than one argument were provided.', | |||
node | |||
}) | |||
} else if (node.arguments.length === 0 && node.parent.property.name !== 'nothing') { | |||
} else if (node.arguments.length === 0 && node.parent?.property?.name !== 'nothing') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the parent-? is not needed, as the CallExpression is always child of something, at least an ExpressionStatement. But it also does not hurt. The property-? fixes it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is indeed unnecessary. I've removed the optional chaining operator after the parent.
ef2c10c
to
f74d579
Compare
🎉 This PR is included in version 4.2.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
The change to the expect-single-argument rule in #373 crashes when you have an expect without a matcher, as pointed out by KayKadner on that PR. By using optional chaining this rule can now handle expect without a subsequent matcher.
How has this been tested?
I've added tests for cases where there is no matcher. These indeed threw an error before applying the fix in this PR.
Types of changes
Checklist
npm run test
and everything passes