-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a 'requireForBlockBody' modifier to the 'arrow-parens' rule (fixes #6557) #6558
Conversation
By analyzing the blame information on this pull request, we identified @alberto, @aubergine10 and @gyandeeps to be potential reviewers |
LGTM |
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. |
👍 |
Marking "do not merge" as issue is not accepted yet. |
Issue is now accepted |
@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! |
@kaicataldo thought you were on it, i can do it as well, just let me know to avoid doing it twice. |
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. |
dfe14ad
to
1d74cfa
Compare
Thanks for the pull request, @nfroidure! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
@kaicataldo done ;). I amended the commit and renamed it, just tell me if i also have to change the branch name. |
1d74cfa
to
8bd0d94
Compare
Thanks for the pull request, @nfroidure! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
@eslintbot For sure, wait a minute ⌚ |
8bd0d94
to
bca8b78
Compare
LGTM |
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! |
* `"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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we don't need this line anymore.
Just a handful of minor tweaks, otherwise LGTM. Thanks @nfroidure! |
@nfroidure Thank you!! I'm looking forward to enabling this rule in Airbnb's linter config :-) |
/*eslint-env es6*/ | ||
|
||
(a) => {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on adding a => ({});
to the correct examples?
Left some suggestions. Thanks so much for working on this @nfroidure! |
@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. |
@kaicataldo everything is well just had no time to dive back in this PR. I'll try to give it a look this week. |
03e00ab
to
44c2012
Compare
LGTM |
@kaicataldo just adapted the PR to your recommendations and rebased:
|
44c2012
to
8606286
Compare
LGTM |
@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 ;) |
|
||
let sourceCode = context.getSourceCode(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra newline
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) => {} ```
8606286
to
a8f96a3
Compare
LGTM |
@kaicataldo feel free to merge if you think this is ready. |
LGTM. Thanks for contributing, @nfroidure! |
Was a great pleasure to help on a product i use on a daily basis. Keep up the good work guys 😘 |
The when-brace option allows to require parens for arrow functions whose body is
wrapped into curly braces like the following:
Otherwise, its behavior remains identical then with the as-needed option.