Add a 'requireForBlockBody' modifier to the 'arrow-parens' rule (fixes #6557) #6558

nfroidure · 2016-06-29T13:11:49Z

The when-brace option allows to require parens for arrow functions whose body is
wrapped into curly braces like the following:

 (a) => {}

Otherwise, its behavior remains identical then with the as-needed option.

mention-bot · 2016-06-29T13:11:51Z

By analyzing the blame information on this pull request, we identified @alberto, @aubergine10 and @gyandeeps to be potential reviewers

eslintbot · 2016-06-29T13:11:51Z

LGTM

jquerybot · 2016-06-29T13:11:53Z

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

SebastienElet · 2016-06-29T15:03:00Z

👍

platinumazure · 2016-06-29T15:30:11Z

Marking "do not merge" as issue is not accepted yet.

kaicataldo · 2016-07-18T20:24:28Z

Issue is now accepted

kaicataldo · 2016-07-21T14:40:28Z

@nfroidure Thanks for contributing to ESLint! We're ready to start moving on this now that the enhancement has been discussed and accepted. The team ended up deciding on a different configuration option than the one you have here - would you be willing to update the PR so that it reflects the issue? Let me know if you have any questions!

nfroidure · 2016-07-21T15:34:48Z

@kaicataldo thought you were on it, i can do it as well, just let me know to avoid doing it twice.

kaicataldo · 2016-07-21T15:56:36Z

Ah, understand the confusion. As a champion for this enhancement it's up to me to either implement or to help someone else implement it. We'd love your contribution, if you're willing :)

If you don't have the time or are not interested, I can definitely take this on, though.

eslintbot · 2016-07-22T08:29:13Z

Thanks for the pull request, @nfroidure! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

nfroidure · 2016-07-22T08:32:13Z

@kaicataldo done ;). I amended the commit and renamed it, just tell me if i also have to change the branch name.

eslintbot · 2016-07-22T08:39:47Z

Thanks for the pull request, @nfroidure! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

nfroidure · 2016-07-22T14:26:21Z

@eslintbot For sure, wait a minute ⌚

eslintbot · 2016-07-22T14:30:17Z

LGTM

kaicataldo · 2016-07-22T14:37:06Z

No worries on the branch name :) The team will start reviewing and we'll leave comments here, should we feel anything needs to be changed/updated. Haven't had a chance to look at the PR yet but will try to do so ASAP. Thanks for contributing!

btmills · 2016-07-23T21:30:10Z

docs/rules/arrow-parens.md

+* `"always"` (default) requires parens in all cases.
+* `"as-needed"` allows ommitting parens in some cases.
+
+ This rule has an object option for exceptions to the `"as-needed"` option:

 You can set the option in configuration like this:


Looks like we don't need this line anymore.

btmills · 2016-07-23T21:41:06Z

Just a handful of minor tweaks, otherwise LGTM. Thanks @nfroidure!

ljharb · 2016-07-23T21:58:04Z

@nfroidure Thank you!! I'm looking forward to enabling this rule in Airbnb's linter config :-)

kaicataldo · 2016-07-25T14:35:56Z

docs/rules/arrow-parens.md

+/*eslint-env es6*/
+
+(a) => {};


Thoughts on adding a => ({}); to the correct examples?

kaicataldo · 2016-07-25T15:03:45Z

Left some suggestions. Thanks so much for working on this @nfroidure!

kaicataldo · 2016-07-31T21:30:31Z

@nfroidure Just wanted to check in and see if there's anything I can do to help get this landed. Looks like we need to rebase and resolve a merge conflict as well.

nfroidure · 2016-08-01T04:56:10Z

@kaicataldo everything is well just had no time to dive back in this PR. I'll try to give it a look this week.

eslintbot · 2016-08-02T07:47:41Z

LGTM

nfroidure · 2016-08-02T07:47:59Z

@kaicataldo just adapted the PR to your recommendations and rebased:

about the duplicate it already was on the previous tests and was not sure of its usefulness. I removed both.
i added a test for the () => ({}) case.
was not aware of the BlockStatement node type and its implications, just replaced it and it actually fixed the edge case you pointed out, nice catch ;).
both {a, b} => a + b and [a, b] => a + b leads to a syntax error so i did not test them. Just added some tests to check ({a, b}) => a + b and ([a, b]) => a + b as valid.

eslintbot · 2016-08-02T08:06:18Z

LGTM

nfroidure · 2016-08-02T08:07:18Z

@ljharb i pushed it before rebasing to give you the opportunity to review the changes before the rebase but it appeared to be a minor conflict ;)

kaicataldo · 2016-08-03T02:48:32Z

lib/rules/arrow-parens.js


        let sourceCode = context.getSourceCode();

+


Extra newline

kaicataldo · 2016-08-03T02:55:56Z

Looks like a rebase is needed, but otherwise this is looking great to me.

…nt#6557) The `'as-needed'` option can now be modified with the `{requireForBlockBody: true}` option allowing to require parentheses for arrow functions whose body is wrapped into curly braces like the following: ```js (a) => {} ```

eslintbot · 2016-08-03T06:45:41Z

LGTM

nzakas · 2016-08-03T17:35:30Z

@kaicataldo feel free to merge if you think this is ready.

kaicataldo · 2016-08-03T19:34:05Z

LGTM. Thanks for contributing, @nfroidure!

nfroidure · 2016-08-04T06:53:48Z

Was a great pleasure to help on a product i use on a daily basis. Keep up the good work guys 😘

jquerybot added CLA: Error and removed CLA: Error labels Jun 29, 2016

platinumazure added the do not merge This pull request should not be merged yet label Jun 29, 2016

kaicataldo removed the do not merge This pull request should not be merged yet label Jul 18, 2016

nfroidure force-pushed the arrow-parens-brace branch from dfe14ad to 1d74cfa Compare July 22, 2016 08:29

nfroidure changed the title ~~Update: Add 'when-brace' option to 'arrow-parens' rule (fixes #6557)~~ Add a 'requireForBlockBody' modifier to the 'arrow-parens' rule (fixes #6557) Jul 22, 2016

nfroidure force-pushed the arrow-parens-brace branch from 1d74cfa to 8bd0d94 Compare July 22, 2016 08:39

nfroidure force-pushed the arrow-parens-brace branch from 8bd0d94 to bca8b78 Compare July 22, 2016 14:30

btmills reviewed Jul 23, 2016
View reviewed changes

kaicataldo reviewed Jul 25, 2016
View reviewed changes

nfroidure force-pushed the arrow-parens-brace branch from 03e00ab to 44c2012 Compare August 2, 2016 07:47

nfroidure force-pushed the arrow-parens-brace branch from 44c2012 to 8606286 Compare August 2, 2016 08:06

kaicataldo reviewed Aug 3, 2016
View reviewed changes

lib/rules/arrow-parens.js

let sourceCode = context.getSourceCode();

Copy link

Member

kaicataldo Aug 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra newline

nfroidure force-pushed the arrow-parens-brace branch from 8606286 to a8f96a3 Compare August 3, 2016 06:45

kaicataldo merged commit 30d71d6 into eslint:master Aug 3, 2016

nfroidure deleted the arrow-parens-brace branch August 4, 2016 06:53

daltones mentioned this pull request Aug 25, 2016

Support modifier requireForBlockBody for rule arrow-parens babel/eslint-plugin-babel#90

Closed

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 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 Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a 'requireForBlockBody' modifier to the 'arrow-parens' rule (fixes #6557) #6558

Add a 'requireForBlockBody' modifier to the 'arrow-parens' rule (fixes #6557) #6558

nfroidure commented Jun 29, 2016

mention-bot commented Jun 29, 2016

eslintbot commented Jun 29, 2016

jquerybot commented Jun 29, 2016

SebastienElet commented Jun 29, 2016

platinumazure commented Jun 29, 2016

kaicataldo commented Jul 18, 2016

kaicataldo commented Jul 21, 2016

nfroidure commented Jul 21, 2016

kaicataldo commented Jul 21, 2016 •

edited

eslintbot commented Jul 22, 2016

nfroidure commented Jul 22, 2016

eslintbot commented Jul 22, 2016

nfroidure commented Jul 22, 2016

eslintbot commented Jul 22, 2016

kaicataldo commented Jul 22, 2016

btmills Jul 23, 2016

btmills commented Jul 23, 2016

ljharb commented Jul 23, 2016

kaicataldo Jul 25, 2016

kaicataldo commented Jul 25, 2016

kaicataldo commented Jul 31, 2016

nfroidure commented Aug 1, 2016

eslintbot commented Aug 2, 2016

nfroidure commented Aug 2, 2016

eslintbot commented Aug 2, 2016

nfroidure commented Aug 2, 2016

kaicataldo Aug 3, 2016

kaicataldo commented Aug 3, 2016

eslintbot commented Aug 3, 2016

nzakas commented Aug 3, 2016

kaicataldo commented Aug 3, 2016

nfroidure commented Aug 4, 2016

Add a 'requireForBlockBody' modifier to the 'arrow-parens' rule (fixes #6557) #6558

Add a 'requireForBlockBody' modifier to the 'arrow-parens' rule (fixes #6557) #6558

Conversation

nfroidure commented Jun 29, 2016

mention-bot commented Jun 29, 2016

eslintbot commented Jun 29, 2016

jquerybot commented Jun 29, 2016

SebastienElet commented Jun 29, 2016

platinumazure commented Jun 29, 2016

kaicataldo commented Jul 18, 2016

kaicataldo commented Jul 21, 2016

nfroidure commented Jul 21, 2016

kaicataldo commented Jul 21, 2016 • edited

eslintbot commented Jul 22, 2016

nfroidure commented Jul 22, 2016

eslintbot commented Jul 22, 2016

nfroidure commented Jul 22, 2016

eslintbot commented Jul 22, 2016

kaicataldo commented Jul 22, 2016

btmills Jul 23, 2016

Choose a reason for hiding this comment

btmills commented Jul 23, 2016

ljharb commented Jul 23, 2016

kaicataldo Jul 25, 2016

Choose a reason for hiding this comment

kaicataldo commented Jul 25, 2016

kaicataldo commented Jul 31, 2016

nfroidure commented Aug 1, 2016

eslintbot commented Aug 2, 2016

nfroidure commented Aug 2, 2016

eslintbot commented Aug 2, 2016

nfroidure commented Aug 2, 2016

kaicataldo Aug 3, 2016

Choose a reason for hiding this comment

kaicataldo commented Aug 3, 2016

eslintbot commented Aug 3, 2016

nzakas commented Aug 3, 2016

kaicataldo commented Aug 3, 2016

nfroidure commented Aug 4, 2016

kaicataldo commented Jul 21, 2016 •

edited