New: Adds implicit-arrow-linebreak rule (refs #9510) #9629

sharmilajesupaul · 2017-11-15T23:16:56Z

What is the purpose of this pull request? (put an "X" next to item)

adds a new rule

What changes did you make? (Give an overview)
Introduces a new rule enforcing the location of arrow function bodies with implicit returns. Also adds tests and docs.
Refs: #9510 & #6067

Is there anything you'd like reviewers to focus on?
I think this rule gets a little less intuitive when it comes to function bodies surrounded by parentheses.

I would like to make sure that this rule never warns on a case where parentheses are used to surround a multiline body, like this:

(foo) => (
  bar
);

However, currently, it does warn on cases like:

// incorrect code for "beside" option
(foo) => 
  (bar);

// incorrect code for "below" option
(foo) => (bar)

I'm not sure if that's the best way to handle this 🤔

cc. @ljharb, @not-an-aardvark
more discussion: #9623.

ljharb

LGTM

kaicataldo · 2017-11-17T20:25:57Z

This needs a champion as well as support from one more team member.

Something to note is that this rule is incompatible with arrow-spacing when this rule is configured with "below" and arrow-spacing is configured with { after: false }. Not an issue since there aren't any cases where they don't overlap.

sharmilajesupaul · 2017-11-18T02:35:34Z

@kaicataldo how do I find someone to champion this rule? Should I make an issue with the rule proposal?

kaicataldo · 2017-11-18T08:14:47Z

@sharmilajesupaul No need to do anything! Thanks for a great PR.

@eslint/eslint-team Looks like we have support from three team members and just need someone to step up to champion.

platinumazure · 2017-11-18T15:37:07Z

I'll champion this. Marking as accepted.

platinumazure

Generally looks good to me. My only concern is possible user confusion about whether arrow-linebreak also applies to arrow functions with block statements. Therefore I would like to suggest that some or all of the following changes be considered:

Add documentation about which rule would control line breaks around braces (brace-style maybe?)
Consider renaming the rule to something like concise-arrow-linebreak or implicit-arrow-linebreak to help make clear that this doesn't apply to non-concise arrow function bodies

Thanks!

ljharb · 2017-11-18T16:00:30Z

"concise" refers to concise object methods; i'd prefer "implicit".

platinumazure · 2017-11-18T16:02:41Z

I've heard it both ways (and I use "shorthand" for those object methods). I'm not going to insist strongly on a name change, but I do think it would help.

not-an-aardvark · 2017-11-18T19:15:35Z

brace-style already covers arrow functions with block statements, so I don't think arrow-linebreak should cover them too.

platinumazure · 2017-11-18T19:21:51Z

brace-style already covers arrow functions with block statements, so I don't think arrow-linebreak should cover them too.

Agreed full stop. However, the rule name and documentation could confuse a new user into thinking that the rule should cover that scenario (until you read some of the examples, anyway). So it makes sense for a See Also section to reference brace-style for sure. I also think it wouldn't hurt to change the name of the rule to be more precise about exactly what it covers.

not-an-aardvark

Thanks for the PR! This mostly looks good to me, I just have a few small suggestions.

not-an-aardvark · 2017-11-18T19:16:45Z

docs/rules/arrow-linebreak.md

@@ -0,0 +1,84 @@
+# Enforce the location of arrow function bodies with implicit returns (arrow-linebreak)
+
+An arrow function body can contain an implicit return as a single statement instead of a block. It can be useful to enforce a consistent location for the arrow function body.


Nitpick: The body of implicit-return arrow functions is actually an expression, not a statement.

not-an-aardvark · 2017-11-18T19:21:34Z

lib/rules/arrow-linebreak.js

+         * @param {string} keywordName The applicable keyword name for the arrow function body
+         * @returns {void}
+         */
+        function validateStatement(node) {


Nitpick: I think this function should be called something like validateExpression rather than validateStatement, because there aren't any statements being used here.

not-an-aardvark · 2017-11-18T19:24:06Z

docs/rules/arrow-linebreak.md

+  return bar();
+}
+
+(foo) => ( // a function body with a leading parentheses is also always allowed with this rule


What's the reasoning for always allowing this? I would have expected that an opening parenthesis on the first line would be treated the same way as anything else on the first line (i.e. allowed with beside, disallowed with below).

Thanks for the review! I originally made this rule to be able to enforce that our code doesn't have lines that end in a =>, but I wanted to make an exception for cases like:

(foo) => ( bar ); // & (foo) => ( bar );

However, I agree with you, treating them the same way will make this rule much clearer. The first case above should be allowed with beside and the second with below. I'll update the logic for this.

not-an-aardvark · 2017-11-18T19:27:50Z

lib/rules/arrow-linebreak.js

+
+            if (hasParens) {
+                fixerTarget = tokenBefore;
+                tokenBefore = sourceCode.getTokenBefore(fixerTarget);


I think this won't be correct if the arrow function body is surrounded by multiple sets of parentheses:

var x = foo => ((((bar))));

In that case, tokenBefore will refer to the wrong thing because it will still be an opening paren.

Instead, one option would be to use some thing like

sourceCode.getTokenBefore(node.body, token => token.value !== "(")

...which will get the first token before the body that isn't an opening paren.

Good call, I completely missed this case entirely 😔 Will make an update soon.

Adds a new rule that enforces consistency of arrow function bodies that contain an implicit return.

sharmilajesupaul · 2017-11-20T02:47:10Z

Made some improvements:

changes rule name from arrow-linebreak -> implicit-arrow-linebreak
adds doc section for brace-style rule (here)
warns on multiline expressions wrapped in parens (see: New: Adds implicit-arrow-linebreak rule (refs #9510) #9629 (comment))
fixes the autofix for expressions wrapped in multiple sets of parentheses i.e. () => ((foo)); (here)
improves variable names, adds new tests, and general clean up

ptal @not-an-aardvark, @platinumazure, & @ljharb, thanks!

gyandeeps · 2017-11-22T22:42:20Z

lib/rules/implicit-arrow-linebreak.js

+                fixerTarget = sourceCode.getTokenAfter(tokenBefore);
+            }
+
+            if (tokenBefore.loc.end.line === fixerTarget.loc.start.line && option === "below") {


nit: I would check for the option first so that we can short-circuit quicker. Same applies below also

gyandeeps · 2017-11-22T22:43:27Z

lib/rules/implicit-arrow-linebreak.js

+        // Public
+        //----------------------------------------------------------------------
+        return {
+            ArrowFunctionExpression: node => validateExpression(node)


directly use the function here

ArrowFunctionExpression: validateExpression

gyandeeps

lgtm. thanks.
just 2 nitpicks

not-an-aardvark

Thanks, this looks good to me. I just have a few minor nitpicks.

not-an-aardvark · 2017-11-22T23:06:10Z

docs/rules/implicit-arrow-linebreak.md

+Examples of **incorrect** code for this rule with the default `"beside"` option:
+
+```js
+/* eslint arrow-linebreak: ["error", "beside"] */


The rule has been renamed to implicit-arrow-linebreak, but this comment still calls it arrow-linebreak. (There are a few other comments like it in this file.)

not-an-aardvark · 2017-11-22T23:07:18Z

tests/lib/rules/implicit-arrow-linebreak.js

+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
+
+ruleTester.run("arrow-linebreak", rule, {


The rule has been renamed to implicit-arrow-linebreak, but this line still calls it arrow-linebreak.

platinumazure · 2017-11-25T00:02:33Z

@sharmilajesupaul I hope it's okay that I fixed some of the last "arrow-linebreak" references on your branch. Of course I know you were more than capable of doing it, but there's a (slim) chance we might still release this weekend despite the US holiday and I wanted to save you some time. Thanks again for contributing!

EDIT: And I renamed the PR title for the same reason. Thanks for your understanding!

not-an-aardvark · 2017-11-26T02:36:03Z

Thanks for contributing!

sharmilajesupaul force-pushed the sj--arrow-linebreak branch from 6203a23 to fc7bbca Compare November 15, 2017 23:28

ljharb approved these changes Nov 16, 2017

View reviewed changes

aladdin-add added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules labels Nov 16, 2017

platinumazure self-assigned this Nov 18, 2017

platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 18, 2017

platinumazure reviewed Nov 18, 2017

View reviewed changes

not-an-aardvark suggested changes Nov 18, 2017

View reviewed changes

New: Adds arrow-linebreak rule (refs eslint#9510)

cedf6b2

Adds a new rule that enforces consistency of arrow function bodies that contain an implicit return.

sharmilajesupaul force-pushed the sj--arrow-linebreak branch from fc7bbca to cedf6b2 Compare November 20, 2017 02:39

ljharb approved these changes Nov 20, 2017

View reviewed changes

gyandeeps reviewed Nov 22, 2017

View reviewed changes

gyandeeps approved these changes Nov 22, 2017

View reviewed changes

not-an-aardvark approved these changes Nov 22, 2017

View reviewed changes

platinumazure added 2 commits November 24, 2017 17:58

Docs: Use "implicit-arrow-linebreak" consistently

0775613

Chore: Use name "implicit-arrow-linebreak"

c939c29

platinumazure changed the title ~~New: Adds arrow-linebreak rule (refs #9510)~~ New: Adds implicit-arrow-linebreak rule (refs #9510) Nov 25, 2017

not-an-aardvark merged commit 4118f14 into eslint:master Nov 26, 2017

renovate bot mentioned this pull request Feb 14, 2018

Update dependency eslint to v4 Urigo/tortilla#62

Merged

eslint-deprecated bot locked and limited conversation to collaborators May 26, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Adds implicit-arrow-linebreak rule (refs #9510) #9629

New: Adds implicit-arrow-linebreak rule (refs #9510) #9629

sharmilajesupaul commented Nov 15, 2017

ljharb left a comment

kaicataldo commented Nov 17, 2017

sharmilajesupaul commented Nov 18, 2017

kaicataldo commented Nov 18, 2017

platinumazure commented Nov 18, 2017

platinumazure left a comment

ljharb commented Nov 18, 2017

platinumazure commented Nov 18, 2017

not-an-aardvark commented Nov 18, 2017

platinumazure commented Nov 18, 2017

not-an-aardvark left a comment

not-an-aardvark Nov 18, 2017

not-an-aardvark Nov 18, 2017

not-an-aardvark Nov 18, 2017

sharmilajesupaul Nov 19, 2017

not-an-aardvark Nov 18, 2017

sharmilajesupaul Nov 19, 2017

sharmilajesupaul commented Nov 20, 2017

gyandeeps Nov 22, 2017

gyandeeps Nov 22, 2017

gyandeeps left a comment

not-an-aardvark left a comment

not-an-aardvark Nov 22, 2017

not-an-aardvark Nov 22, 2017

platinumazure commented Nov 25, 2017 •

edited

not-an-aardvark commented Nov 26, 2017

		@@ -0,0 +1,84 @@
		# Enforce the location of arrow function bodies with implicit returns (arrow-linebreak)

		An arrow function body can contain an implicit return as a single statement instead of a block. It can be useful to enforce a consistent location for the arrow function body.


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

		ruleTester.run("arrow-linebreak", rule, {

New: Adds implicit-arrow-linebreak rule (refs #9510) #9629

New: Adds implicit-arrow-linebreak rule (refs #9510) #9629

Conversation

sharmilajesupaul commented Nov 15, 2017

ljharb left a comment

Choose a reason for hiding this comment

kaicataldo commented Nov 17, 2017

sharmilajesupaul commented Nov 18, 2017

kaicataldo commented Nov 18, 2017

platinumazure commented Nov 18, 2017

platinumazure left a comment

Choose a reason for hiding this comment

ljharb commented Nov 18, 2017

platinumazure commented Nov 18, 2017

not-an-aardvark commented Nov 18, 2017

platinumazure commented Nov 18, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharmilajesupaul commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyandeeps left a comment

Choose a reason for hiding this comment

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure commented Nov 25, 2017 • edited

not-an-aardvark commented Nov 26, 2017

platinumazure commented Nov 25, 2017 •

edited