Fix prefer-explicit-assert raising error on destructuring statement

[afontcu]

Closes #40.

I've added a check for Array destructuring. I've never seen it before, but I thought it wouldn't hurt.

Thanks for your help!

[thomaslombart]

I don't think checking for VariableDeclarator is necessary, is it?

[afontcu]

oh, I see, it's not. Fixed!

This made me think of something else, though. Should the following line be considered as a valid one? Right now it is consideren invalid:

const a = [ getByText('foo'), 'bar' ]

[thomaslombart]

That's interesting... I guess it should be considered as valid since we don't use it as an assertion here. We need to fix that as well.

Thanks for spotting the issue! 🙂

[afontcu]

okay, I'll work on that and update the PR accordingly! :)

[thomaslombart]

All right! Thanks for working on that 😄

[afontcu]

Okay, just updated the PR. I think checking if grandparent is an ArrayExpression will suffice, but please do doublecheck it as I'm relying on tests I wrote and the AST explorer which is just learned about when you shared the screenshots 😇

thanks for helping me out!

[thomaslombart]

It's sufficient for array declarations but I spotted the same issue if you declare an object, for example:

const hello = { foo: getByText("bar") }

In the end, I'm not sure if checking for array or object declaration is the right approach. I think we should just check if getByText is both being called and assigned to a variable. Thus, we would need to do some modifications to isDeclared.

[thomaslombart]

I got it working by doing this:

1. Add a findParent function in the utils file:

const findParent = (node, cb) => {
  if (cb(node)) {
    return node;
  } else if (node.parent) {
    return findParent(node.parent, cb);
  } else {
    return null;
  }
};

2. Modify both isDeclared and findCallExpressionParent:

const findCallExpressionParent = node =>
  findParent(node, node => node.type === 'CallExpression');

// ... 

const isDeclared = node => {
  return !!findParent(node, node => node.type === 'VariableDeclarator');
};

3. I removed the isInArrayDeclaration function

4. I added a last test:

    {
      code: `const a = { foo: getByText('bar') }`,
    },

That way, we don't dive into specific details of how getBy queries have been declared, we just check if one parent (no matter where in the tree) has a type of VariableDeclarator. I think it should be good by doing so 🙂

[afontcu]

It looks way smarter to check for positive cases rather than trying to take into account every posssible wrong edge case. Updating the PR.

[Meemaw]

Any estimates on when this will be merged? Great work regardless! 👍

[afontcu]

Any estimates on when this will be merged? Great work regardless! 👍

AFAICT it's ok from my side, let's see if Thomas can take a look :)

[thomaslombart]

Hey there! Sorry, I have less time to check the PRs. I think it should be fixed by tomorrow 🙂

[thomaslombart]

All LGTM now! 🎉
Thanks a lot for your contribution @afontcu, I really appreciate it 😄

[Belco90]

🎉 This PR is included in version 1.3.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[afontcu]

Thank you for guiding me through!

[thomaslombart]

@all-contributors please add @afontcu for code and test.

[allcontributors]

@thomlom

I've put up a pull request to add @afontcu! 🎉