Add `no-inline-assertions` rule #262

JLHwung · 2019-06-13T02:29:21Z

Update (2019-06-13): I have pushed no-inline-assertions rule implementation, see the docs and test cases. This rule will fail

Any MemberExpression as the arrow function body

test('foo', t => t.true(fn()));
test('foo', t => t.skip());
test('foo', t => 
    t.true(fn())
);
// ...

And any BlockStatement within the same line

test('foo', t => { t.true(fn()) });
test('foo', t => { t.skip() });

(2019-06-12)
This is an early-stage design discussion, as I have the following question regards to this rule. I would also put up my thoughts and any community input is welcome.

Background: Recap of #152

The no-inline-assertions was originally proposed at #152, where Sindre listed two example

Pass

test('foo', t => t.true(fn()));

Failure

test('foo', t => {
    t.true(fn());
});

Question on extra examples

While the example above is unambiguous, there is more question to be addressed after I ponder for a while.

Should inline arrow function with block statement be failed?

test('foo', t => { t.true(fn()) });

Should inline arrow function with a return statement be failed?

test('foo', t => { return t.true(fn()) });

Should inline arrow function with assertion call as last expression be failed?

test('foo', t => { t.log('Meow');t.true(fn()) });

Here are my thoughts on these cases,
On the first, I think it should be failed as an inline arrow function with block statement is still inline.
On the second one, I think there should be a separate rule, e.g. no-return-assertions to take care of this. Otherwise we will have also to fail the following case

test('foo', t => {
  return t.true(fn());
});

which is, of course, not an inline arrow function. So rule no-inline-assertions should not be triggered by case 2.

On the third one, I think it should be failed as it is still an inline arrow function with assertion calls inside. But I am not an ava user so I can't triage the impact of the third case. Is it common? Or we can neglect this case?

The docs here reflects my thoughts and is suggested to any changes.

Fixes #152

GMartigny · 2019-06-13T10:47:41Z

I think the 2nd example fall under the arrow-body-style rule of eslint.

IMO, this no-inline-assertion should fail no matter what, when test second param is in the same line.

JLHwung · 2019-06-14T00:31:01Z

IMO, this no-inline-assertion should fail no matter what, when test second param is in the same line.

Agree. I was worrying about test("somethigs", (t) => { t.skip() }); before but since there is an ava/use-t-well rule, it should be straightforward to fail when the block statement is a one-liner.

I suggest there be an exception:

When there is an empty test body

test("something", (t) => {});

we should not fail as it may be an IDE auto-completed scaffold. And developer may leave it as is when writing test to drive development.

GMartigny · 2019-06-16T20:19:08Z

When there is an empty test body, we should not fail

I agree, no body is not an inline body.

You forgot to add the rule to recommended config in index.js.

docs/rules/no-inline-assertions.md

readme.md

sindresorhus · 2019-07-01T16:38:35Z

Should inline arrow function with block statement be failed?

That's not an inline arrow function. And no.

Should inline arrow function with a return statement be failed?

That's not an inline arrow function. And no, I think this would be a better fit for the use-t-well rule. It could validate that no assertion is returned, unless it's an assertion that returns a promise, in which, it's allowed.

Should inline arrow function with assertion call as last expression be failed?

That's not an inline arrow function. And no.

sindresorhus · 2019-07-01T16:39:55Z

IMO, this no-inline-assertion should fail no matter what, when test second param is in the same line.

I disagree. That's better handled by normal ESLint style rules. The point here is to handle an inline arrow function, meaning x => x (not x => { return x }).

sindresorhus · 2019-07-01T16:41:34Z

rules/no-inline-assertions.js

+function report({node, context}) {
+	context.report({
+		node,
+		message: 'Assertions should not be called from an inline function.'


This would be more correct:

Suggested change

message: 'Assertions should not be called from an inline function.'

message: 'The test implementation should not be just an assertion in an inline arrow function.'

But would be nice to find a way to shorten the above slightly.

JLHwung · 2019-07-01T17:00:17Z

The point here is to handle an inline arrow function, meaning x => x (not x => { return x }).

Per https://tc39.es/ecma262/#prod-ConciseBody, we should alias our definition of "inline arrow function" to "arrow function with concise body", and then we can clearly state that x => { return x } is not an inline arrow function.

And the error message could be

The test implementation should not be an inline arrow function.

If we agree that BlockStatement should not be considered inline, we should leave it as-is and only take care of CallExpression.

sindresorhus · 2019-07-01T17:11:41Z

The test implementation should not be an inline arrow function.

👍

If we agree that BlockStatement should not be considered inline, we should leave it as-is and only take care of CallExpression.

👍

JLHwung · 2019-07-01T18:50:10Z

@sindresorhus Just pushed a new version and revised test cases.

docs/rules/no-inline-assertions.md

sindresorhus · 2019-07-03T16:15:32Z

docs/rules/no-inline-assertions.md

@@ -0,0 +1,19 @@
+# Ensure no assertions are called from inline functions
+
+Prevent assertions being called from an inline function, to make it clear that it does not return.


This is repeating the title too much. It should also use the wording "test implementation".

test/no-inline-assertions.js

sindresorhus · 2019-07-03T16:23:42Z

Can you make the rule auto-fixable?

Co-Authored-By: Sindre Sorhus <[email protected]>

JLHwung · 2019-07-03T17:50:56Z

Can you make the rule auto-fixable?

Good idea, it could be autofixable and I would look into it later. Anyway I have fixed the writing issues.

JLHwung · 2019-07-03T21:11:31Z

@sindresorhus I have pushed a simple fixer. The fixer wraps callExpression inside brackets. i.e. () => x() will be fixed to () => {x()}.

I tend to keep the fixer as minimal as possible, which means the other ESLint style rules, i.e. prettier should take care of the formatting of the new BlockStatement, such as the extra space before or after brackets or potential line breaks around the brackets.

sindresorhus · 2019-07-04T15:05:30Z

You need to mark it as being fixable in the readme and the rule docs.

sindresorhus · 2019-07-04T15:05:50Z

I have pushed a simple fixer. The fixer wraps callExpression inside brackets. i.e. () => x() will be fixed to () => {x()}.

👍 Yup. That's the correct way to do it.

readme.md

docs/rules/no-inline-assertions.md

sindresorhus · 2019-07-05T04:59:32Z

You need to mark it as being fixable in the readme and the rule docs.

Not resolved yet.

sindresorhus · 2019-07-05T05:04:28Z

This looks good now. Thanks for working on this rule :)

docs: add no-inline-assertions rule example

bbc96b6

JLHwung changed the title ~~WIP: Add no-inline-assertion rule (Fixes #152)~~ WIP: Add no-inline-assertions rule (Fixes #152) Jun 13, 2019

JLHwung added 2 commits June 13, 2019 21:17

docs: add more invalid examples

1ecdbdb

feat: no-inline-assertions rule

c604850

JLHwung changed the title ~~WIP: Add no-inline-assertions rule (Fixes #152)~~ Add no-inline-assertions rule (Fixes #152) Jun 14, 2019

JLHwung added 2 commits June 13, 2019 21:28

refactor: make xo happy

6d79c90

docs: add rule link to readme

8e14e81

JLHwung force-pushed the no-inline-assertions branch from 4c3b778 to 8e14e81 Compare June 14, 2019 01:50

novemberborn requested a review from sindresorhus June 16, 2019 14:39

feat: add no-inline-assertions to recommended config

8d36135

GMartigny approved these changes Jun 17, 2019

View reviewed changes

fix: missing trailing period

35c1665

JLHwung commented Jun 17, 2019

View reviewed changes

docs/rules/no-inline-assertions.md Outdated Show resolved Hide resolved

docs: remove dummy link

75cfe5b

sindresorhus reviewed Jul 1, 2019

View reviewed changes

readme.md Outdated Show resolved Hide resolved

docs: align title

c85588b

sindresorhus changed the title ~~Add no-inline-assertions rule (Fixes #152)~~ Add no-inline-assertions rule Jul 1, 2019

sindresorhus reviewed Jul 1, 2019

View reviewed changes

feat: ignore BlockStatement

fab3dc8

JLHwung and others added 2 commits July 2, 2019 08:25

docs: update fail examples per new behavior

b44c571

Update no-inline-assertions.js

f23fd3b

sindresorhus requested changes Jul 3, 2019

View reviewed changes

JLHwung and others added 2 commits July 3, 2019 13:23

Update docs/rules/no-inline-assertions.md

a7f78b3

Co-Authored-By: Sindre Sorhus <[email protected]>

docs: fix writing issues

b07ccb5

feat: add fixer to no-inline-assertions

69144d8

JLHwung force-pushed the no-inline-assertions branch from 4160751 to 69144d8 Compare July 3, 2019 21:05

Update no-inline-assertions.md

aa7752e

sindresorhus requested changes Jul 4, 2019

View reviewed changes

readme.md Outdated Show resolved Hide resolved

docs: no-inline-assertions is fixable

d2f49a4

JLHwung commented Jul 5, 2019

View reviewed changes

docs/rules/no-inline-assertions.md Show resolved Hide resolved

sindresorhus added 2 commits July 5, 2019 12:02

Update no-inline-assertions.md

346145b

Update readme.md

a5377cd

sindresorhus merged commit b829dbc into avajs:master Jul 5, 2019

JLHwung deleted the no-inline-assertions branch July 5, 2019 11:58

	message: 'Assertions should not be called from an inline function.'
	message: 'The test implementation should not be just an assertion in an inline arrow function.'

		@@ -0,0 +1,19 @@
		# Ensure no assertions are called from inline functions

		Prevent assertions being called from an inline function, to make it clear that it does not return.

Add no-inline-assertions rule #262

Add no-inline-assertions rule #262

Uh oh!

Conversation

JLHwung commented Jun 13, 2019 • edited by sindresorhus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background: Recap of #152

Pass

Failure

Question on extra examples

Uh oh!

GMartigny commented Jun 13, 2019

Uh oh!

JLHwung commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GMartigny commented Jun 16, 2019

Uh oh!

Uh oh!

Uh oh!

sindresorhus commented Jul 1, 2019

Uh oh!

sindresorhus commented Jul 1, 2019

Uh oh!

sindresorhus Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

JLHwung commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sindresorhus commented Jul 1, 2019

Uh oh!

JLHwung commented Jul 1, 2019

Uh oh!

Uh oh!

sindresorhus Jul 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sindresorhus commented Jul 3, 2019

Uh oh!

JLHwung commented Jul 3, 2019

Uh oh!

JLHwung commented Jul 3, 2019

Uh oh!

sindresorhus commented Jul 4, 2019

Uh oh!

sindresorhus commented Jul 4, 2019

Uh oh!

Uh oh!

Uh oh!

sindresorhus commented Jul 5, 2019

Uh oh!

sindresorhus commented Jul 5, 2019

Uh oh!

Uh oh!

Add `no-inline-assertions` rule #262

Add `no-inline-assertions` rule #262

JLHwung commented Jun 13, 2019 •

edited by sindresorhus

Loading

JLHwung commented Jun 14, 2019 •

edited

Loading

JLHwung commented Jul 1, 2019 •

edited

Loading