Skip to content
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

Update configuration.js #7

Merged
merged 5 commits into from
May 27, 2022
Merged

Update configuration.js #7

merged 5 commits into from
May 27, 2022

Conversation

mordamax
Copy link
Contributor

Fix #6

@mordamax mordamax requested a review from a team as a code owner May 17, 2022 12:07
@@ -91,7 +91,7 @@ const baseRules = {
"dot-notation": "error",
"no-redeclare": "error",
"arrow-parens": "error",
"arrow-body-style": ["error", "always"],
"arrow-body-style": ["error", "as-needed"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer it as "always" because I don't like the special syntax needed for blockless arrow functions. For very simple functions (single line) it looks somewhat fine but in other cases it makes the code looks inconsistent with the rest of the code that doesn't need special syntax and special semantics (implicit return).

From my experience those are their downsides

  1. It's harder to debug functions without a block. e.g. If I have
foo.bar((baz) => {
  // code
})

I can simply

foo.bar((baz) => {
+  debugger // or console.log(...)
  // code
})

But if I have foo.bar((baz) => foo(baz)) it's more difficult to write a debugger/print statement - I'd have to do a mini-refactor of the code for that and revert it after. Using "bodyless" style makes it more cumbersome to quickly debug something and I don't think the "readabilty" gains are worth it.

  1. Object literal looks less readable as showcased by how getTypescriptOverride changed because the { for opening the object is glued to the (.

So in my opinion "bodyless" only looks good in the .foo(val => short) expressions and terrible/inconsistent everywhere else, hence why it's as "always" right now.

I'll ask for a third opinion here from @mutantcornholio.

If we're going with as-needed I'll vote for going with { "requireReturnForObjectLiteral": true } (https://eslint.org/docs/rules/arrow-body-style#requirereturnforobjectliteral)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really miss to see these nice shorthands... aren't they worth adding few curly braces sometimes to debug? ))
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
image

isn't it too verbose for such straighforward operations?

Copy link
Contributor

Choose a reason for hiding this comment

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

And I would actually prefer as-needed without requireReturnForObjectLiteral :D
It's a matter of habit and aesthetics, so I sure can live with other options

As all options have their pros and cons, should we pick the option with least changes to make?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or wait, that's actually what @mordamax originally suggested, right?

Do we want to vote on requireReturnForObjectLiteral separately?

Copy link
Contributor

@joao-paulo-parity joao-paulo-parity May 17, 2022

Choose a reason for hiding this comment

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

Am I right, that { "requireReturnForObjectLiteral": true } will still end up with something like this ?

@mordamax No because that rule only matters for returning object literals. Please check out the example in https://eslint.org/docs/rules/arrow-body-style#requirereturnforobjectliteral.

Copy link
Contributor

Choose a reason for hiding this comment

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

but no one is opposing against it

I do oppose against it.
Not strongly, as it's all codestyle and I could live with any configuration, but that's what we're discussing, right?
For me, requireReturnForObjectLiteral doesn't bring much to the table and forbids me to write the code how I like to write it.

@mordamax I left an example right above your comment. This is a flag that forbids to quickly return object from an arrow function, and would require a function body with a return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as I understand only if you return an object.
in case of expression like this (attached) it works as short-hand

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly. And I'd like to be able to return an object with a simpler syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though ideally i'd like requireReturnForObjectLiteral: false, I think as-needed already good improvement.
So if @joao-paulo-parity is strongly against it, while you're not strongly against it (same as me), I'd end up with requireReturnForObjectLiteral: true as a decent compromise for majority of use-cases of short-hand expressions.
If you agree, lets merge, otherwise, i'd propose to keep "never", as it was before, since it's the way how many projects already have been built

Copy link
Contributor

@joao-paulo-parity joao-paulo-parity left a comment

Choose a reason for hiding this comment

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

Changing my review's status from "requested changed" to "comment" to unblock the merge

Didn't work

@joao-paulo-parity joao-paulo-parity self-requested a review May 17, 2022 14:52
@mordamax mordamax merged commit a5b700e into master May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change arrow-body-style to as-needed
3 participants