chore: add parseExpect() util function

[macklinu]

This is related to #73. I was curious to see what a parseExpect() function looked like and to get some feedback on it. Right now it looks like:

type ParsedExpect = {
  /* The argument nodes passed to `expect()`. */
  arguments: Node[],
  /* The properties chained onto `expect()`, like `.not` or `.resolves`. */
  properties: Node[],
  /* The matcher function called on `expect()`, like `.toBe()`, or undefined if missing. */
  matcher: Node | undefined,
  /* The argument nodes passed to the matcher function. */
  matcherArguments: Node[],
}

/* If a CallExpression node is passed in, return a ParsedExpect object, otherwise null. */
function parseExpect(node: Node | null): ParsedExpect | null

I can see helper functions being built on top of this parsing function, like:

/* The property names chained onto an `expect()` call. */
function expectPropertyNames(expect: ParsedExpect | null): string[] {
  return expect
    ? expect.properties.map(property => property.name)
    : []
}

I can also see this helping simplify some of the logic in the valid-expect rule (and the WIP #78).

Thoughts on this as a starting point? I haven't integrated this function into any of the rules, but I'd be happy to explore that refactoring in this PR if we'd like to see what this API looks like when it's used.

[SimenB]

Ooooh, awesome! I really like this, it's pretty much how I imagined it. Great start!

Thoughts on this as a starting point? I haven't integrated this function into any of the rules, but I'd be happy to explore that refactoring in this PR if we'd like to see what this API looks like when it's used.

Yeah, I think playing around with in in the different rules would be good, to see how it holds up

[macklinu]

I will follow up with a commit or another PR in the next week or two when I have computer access again. 🙂

…

On Mon, Mar 19, 2018 at 3:41 AM Simen Bekkhus ***@***.***> wrote: Ooooh, awesome! I really like this, it's pretty much how I imagined it. Great start! Thoughts on this as a starting point? I haven't integrated this function into any of the rules, but I'd be happy to explore that refactoring in this PR if we'd like to see what this API looks like when it's used. Yeah, I think playing around with in in the different rules would be good, to see how it holds up — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#101 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPEyUZ0ers8Fo8lEFAbI7XAzLgZTrm-ks5tf2EvgaJpZM4Spooy> .

[macklinu]

I have a computer set up for programming again, will follow up today or this week on this PR. 👍

[SimenB]

@macklinu GitHub doesn't send emails for pushes in this repo for some reason... Is this good to go?

[macklinu]

@SimenB it may have been me rebasing this branch and force-pushing back up (for why you didn't receive a notification). This slipped my mind for a bit - will have some time in the next couple weeks to follow up with integrating this util function into a rule. Shall I close this and reopen when it's ready for another review?

[SimenB]

I don't mind it being open if you don't 🙂 Give me a ping when you think it's ready for review, or have any questions.

[macklinu]

I tried to use this util to implement #103 to see how it would work in practice. I think it handles parsing a CallExpression well:

function create(context) {
  return {
    CallExpression(node) {
      const expect = parseExpect(node)
      if (!expect) return
      // do something with the `expect` object
    },
  }
}

However, for the case of #103 and handling a multi option (only ensure .toMatchSnapshot() is called with a name if a test contains multiple .toMatchSnapshot() calls), I'm not sure if this util function handles a case like this (where we have to traverse down the tree and filter out CallExpression statements within a BlockStatement):

// code to lint
test('my test', () => {
  expect(a).toMatchSnapshot('a')
  expect(b).toMatchSnapshot('b')
})

// sample rule
const rule = {
  create(context) {
    return {
      CallExpression(node) {
        if (util.isTestCase(node)) {
          const expectsWithinTestCase = node.arguments[1].body.body.reduce(
            (array, node) => {
              const expect = parseExpect(node.expression);
              return expect ? array.concat(expect) : array;
            },
            []
          );
          console.log(expectsWithinTestCase); // always empty, ideally would be length 2
        }
      },
    };
}

There might be a better way to analyze multiple expect() CallExpression statements within a function body - and if so please let me know - but as of now, this case isn't handled but would be really useful to consider.

[SimenB]

Added in #383