-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Remove unwanted single returns from arrow functions #12127
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
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.
In general I like this change but there are cases where this can reduce readability IMO, so I'm slightly 👎🏻 . Not a big deal though if others feel differently.
@@ -19,19 +19,17 @@ const { CAVEATS, ERRORS, PERMS, RPC_REQUESTS } = getters; | |||
|
|||
const { ACCOUNTS, DOMAINS, PERM_NAMES } = constants; | |||
|
|||
const initPermController = () => { | |||
return new PermissionsController({ | |||
const initPermController = () => |
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.
I feel like in cases like this, the braces can help with readability.
getIdentities: () => { | ||
return keyringAccounts.reduce((identities, address) => { | ||
getIdentities: () => | ||
keyringAccounts.reduce((identities, address) => { |
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.
Same thing with this. It's not impossible to know where the function begins and ends, of course, because of the indentation, but the braces help in being able to spot it more easily.
@@ -52,6 +52,7 @@ module.exports = { | |||
'default-param-last': 'off', | |||
'prefer-object-spread': 'error', | |||
'require-atomic-updates': 'off', | |||
'arrow-body-style': 1, |
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.
Can we use strings instead of numbers here? We've been trying to avoid using numbers so we don't have to remember what they mean.
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.
I tried 'error'
but for some reason the linting bombed, requesting a number. I'll look into it, it was weird.
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.
Update: We can't have nice things: MetaMask/eslint-config#205 (comment)
I'm in favor of this change. I do see @mcmire 's point on readability in some cases, but in my opinion, it can be generally more readable in more circumstances than it is less. The lack of bracket is a clear indication that the function simply returns whatever follows, and doesn't contain any logic of its own. This is my experience, others may feel differently and should voice their opinions. Also, at what point does it make sense to elevate this to the eslint config repo? |
Agree with @darkwing , @brad-decker : to me also this is more readable and kind of simplified. I always prefer not to have brackets around arrow functions whenever possible. |
Closing for upstream MetaMask/eslint-config#205 |
This is a code styling proposal to remove unnecessary
return
statements inside arrow functions when we could use the implied return. For example:The real work is
arrow-body-style: 1
in our ESLint config -- the rest of these changes are done viayarn lint --fix
.Thoughts?