Make no-mocha-arrows rule fixable

[rhysd]

Summary

I make no-mocha-arrows rule fixable with --fix option of eslint command.

Example

• Before

it("does something", () => { assert.ok(true); });
it("does something", () => assert.ok(true));

• After

it("does something", function () { assert.ok(true); });
it("does something", function () { return assert.ok(true); });

EDIT: Translation result of single expression body

[rhysd]

Travis CI workers don't start... Could you restart them?

[jfmengels]

Hi @rhysd! Thanks a lot for this :D

Could you add some additional tests for

• Generator functions (*() => {} if I recall correctly)
• Async functions (async () => {})
• Functions with callbacks ((done) => {})

[jfmengels]

The output should be
it(function () { return assert(something, false); })

Imagine the user wrote a test for function that returns a Promise, and they just want to check that it resolves.

it(() => lib.resolves());

If it's changed to

it(() => {
  lib.resolves();
});

then the test will passed even if the Promise rejects. It's much safer to change it to

it(function() {
  return lib.resolves();
});

[rhysd]

Ah, you're correct. I'll fix this.

[rhysd]

My TODO:

• Add test cases for generator function, async function and done callback
• Missing return statement

[rhysd]

@jfmengels

I fixed all the points. Could you review additional patches?

[jfmengels]

Both async () => {} and async() => {} are actually valid. This would create broken code if you find the latter form. (add a test for that obviously :) )

[rhysd]

Hmm... It looks nice point. I need to improve code transform on async function.

[jfmengels]

Pretty sure you can do

ruleTester = new RuleTester({
  parserOptions: {ecmaVersion: 2017}
});

and remove all the parserOptions override in individual test cases.

[rhysd]

Oh, that's much better. I don't know eslint toolchain very much. Thanks.

[jfmengels]

I usually extract the common errors into a variable and just put it everywhere. Removes a lot of duplicated code. but @lo1tuma may not have the same preference.

var errors =  [ { message: 'Do not pass arrow functions to it()', column: 1, line: 1 } ];

 ruleTester.run('no-mocha-arrows', rules['no-mocha-arrows'], {
 // ...
  {
    code: '...',
    errors: errors
  }
});

[jfmengels]

Since this function is starting to get pretty big, declare it somewhere else and pass the needed info

function fix(context, fnArg, ...) {
  return function (fixer) {
     var sourceCode = context.getSourceCode(),
           paramsLeftParen = sourceCode.getFirstToken(fnArg),
     // ...
  };
}

// ...
context.report({
  node: node,
  message: // ...
  fix: fix(context, fnArg, ...)
});

[rhysd]

I agree :)

[rhysd]

Thank you @jfmengels for nice points. I'll fix them.

[rhysd]

@jfmengels

I added extra 4 commits. Each commit corresponds to your review comment. Could you review them?

[jfmengels]

Apart from this last almost esthetic comment, LGTM :)
Thanks for working on this, and sorry for the late review.

It's up to @lo1tuma to merge afterwards :)

[jfmengels]

Would have been simpler (and less confusing) to have only one ruleTester.run call with the es6 settings, and just override the parser settings where needed (in the following cases).

[rhysd]

I feel it's enough to use test runner using es2017 parser for every test. I'll make one test runner for tests.

[rhysd]

@jfmengels I fixed the point 😄

[jfmengels]

LGTM 🎉

[lo1tuma]

LGTM as well 👍 .

Thanks everyone.