prettier / prettier

Prettier is an opinionated code formatter.
https://prettier.io
MIT License
49.37k stars 4.34k forks source link

Parentheses around single parameter arrow functions / Add support for arrow-parens property #812

Closed DavidBabel closed 6 years ago

DavidBabel commented 7 years ago

Hey !

It will be great to add an option to disable auto remove of parenthesis when there is only one argument in an arrow function. Most of professional projects configure their way since it's the default eslint behaviour. Here is a Eslint Link : http://eslint.org/docs/rules/arrow-parens

// Bad
a => {}

// Good
(a) => {}
andreypopp commented 7 years ago

I think this is needed โ€” always presenting parens make refactoring easier: when you need to add another arg you don't need to add parens (cause they are present already).

DavidBabel commented 7 years ago

Personnally i found the real problem is in the spec, where only the "one args" arrow function allow removing parenthesis. It's handy when you code very small applications, but it's not at all when you need modify your prototype later.

() => {}             // ok
   => {}             // not allowed
(arg1) => {}         // ok
 arg1  => {}         // ok
(arg1, arg2) => {}   // ok
 arg1, arg2  => {}   // not allowed

If it's not configurable in prettier and the default configuration of prettier is opposite with default behaviour of ESlint. It makes prettier unusable for existing projects unless changing all project ESlint rules, and it's a huge problem on multi users projects.

vramana commented 7 years ago

Except in extreme cases, we are not going to add extra configuration options. The question then is can we make parenthesis around arrow function arg the default. Here is @jlongster's reply in another thread in case you missed it.

Single-arg arrow functions are also extremely common and I don't think it's worth adding parens just to make it easier to add args later. It's really nice to just have arr.map(x => ...).

https://github.com/prettier/prettier/issues/572#issuecomment-277049560

Therefore I am closing this issue.

jlongster commented 7 years ago

I realize this is one of those things where you can't please both sides. :( Many people would not like this change. We try to make decisions based on what a majority of people want, not just based on our own personal styles, but sometimes "majority" is hard to define.

rhengles commented 7 years ago

@DavidBabel I just added this option in https://github.com/arijs/prettier-with-tabs/commit/0f918465dc1d16f8826268c658f55a27af7eb963

DavidBabel commented 7 years ago

@rhengles awesome

androa commented 7 years ago

Sorry for digging up this one, but I just implemented support for the --arrow-parens option with the values always, as-needed, and requireForBlockBody (to replicate eslint arrow-parens). I should probably have searched for this issue first though :stuck_out_tongue_winking_eye:

The current behaviour would then be "as-needed".

We run Airbnb's eslint config (as very many others), and this is the only thing that blocks us from just applying prettier all over.

Is this still regarded as a non-extreme case (ref. https://github.com/prettier/prettier/issues/812#issuecomment-282752234), or is this something worth finishing and PR?

drewhamlett commented 7 years ago

Same here. The biggest hold up for us is this rule arrow-parens. Quite a few people use Airbnb styleguide and lining up most stuff with that project would be awesome. I've tried prettier-eslint but the performance of eslint --fix is not that good.

dejanr commented 7 years ago

This is exactly why i got here also. Our team is aligned with airbnb styleguide. And after trying prettier, this is the rule preventing us to use prettier.

As far as i know its not possible to exclude this rule from prettier?

rmoorman commented 7 years ago

Seconded. We are employing arrow-parens with always as a rule too and would really like to use prettier. Wouldn't it be possible to reconsider this change (to opt-in on keeping the parens through configuration options) given that there is even something that could be forged into a PR @jlongster and @vramana?

drewhamlett commented 7 years ago

I hate the idea of options in the formatter but I wish this lined up more with Airbnb styleguide. Again eslint --fix does solve this but the performance is really bad.

MikeyBurkman commented 7 years ago

I'm surprised no one has mentioned how this enforces inconsistent formatting: "Require X except when Y". Why wouldn't you prefer "Always require X" if it's always possible?

(Also, to me, this is kind of similar to the escaping quotes issue #973, adding inconsistencies while claiming that it makes it easier to read.)

androa commented 7 years ago

I'm surprised no one has mentioned how this enforces inconsistent formatting

I believe that is a discussion that belongs on the styleguide defined by Airbnb, which is mostly a de facto industry standard by now.

While I can understand that some disagrees with the styleguide presented by Airbnb, I don't think it should be prettiers goal to enforce different standards. Staying aligned with Airbnb is crucial in order to be adopted by teams who already employ the Airbnb standard (which is quite a lot of teams).

drewhamlett commented 7 years ago

This is just as inconsistent.

foo(param => {
  console.log(param)
})

Now when I need another param which happens all the time I have to add the parens.

foo((param, param2) => {
  console.log(param)
})

With the Airbnb style I can quickly glance an arrow function and know if its' an implicit return or not.

foo(i => i.name)
foo ((i) => { console.log(i)}

To me this is super helpful.

Anyway I've switched from Sublime to Atom and the speed slowdown using eslint --fix with prettier isn't noticeable. Hopefully I can get used to the font rendering. I think the prettier package does some kind of memoization of the function since you're in an actual node environment. In Sublime you boot node every time.

burabure commented 7 years ago

We try to make decisions based on what a majority of people want, not just based on our own personal styles, but sometimes "majority" is hard to define.

@jlongster I don't think majority is hard to define in this case. I completely understand the strictness when introducing config options, but It seems this one deserves consideration.

image

image

image

dlindenkreuz commented 7 years ago

@vjeux @jlongster Any final words from the masterminds?

My 2ยข: having a configurable option would matter to me as a TypeScript user. For me, parens by default are not only useful for adding arguments later on but also for adding type annotations.

If you don't want to introduce another configurable option (which is imho super reasonable in regard to the project's philosophy), it would still be a sensible default for all TypeScript files, which would in turn follow the project's philosophy ๐Ÿ˜

(โ€ฆmeaning that parens for single argument arrow fns are only added when prettifying a TypeScript file and not added for JavaScript files)

const foo = x => Math.abs(x) // ok, let's annotate x
const foo = x: number => Math.abs(x) // dang, invalid typescript
const foo = (x: number) => Math.abs(x) // adding them good ol' parens
paldepind commented 7 years ago

If I understand correctly, one of the huge benefits that Prettier gives is that I can write code as sloppy as I please and then rely on Prettier to make it nice and readable.

IMO leaving out parentheses in arrows functions is one of these things that makes code easier to write but harder to read and maintain.

If Prettier added parentheses it would give me the best of both worlds. I could be sloppy and write arrow functions without the parenthesis. And later, when I had to read or expand the code, Prettier would have been there to add them in.

kachkaev commented 7 years ago

Facing a styling deadlock because prettier lacks --arrow-parens option and I'm having 'arrow-parens': ['error', 'always'] in my eslint config. When I use prettier-eslint, prettier first removes the parenthesis but then they are added by eslint --fix. In a project with about 100 js files, this has created three unsolvable max-len errors. Something like:

// original code:                                          โ†“ max-len
const filteredThings=_.map(things,(thing) => thing.x > 0);

// after prettier:
const filteredThings = _.map(things, thing => thing.x > 0);

// after eslint --fix inside prettier-eslint:
const filteredThings = _.map(things, (thing) => thing.x > 0);
//                                                         โ†‘ max-len eslint error!

Shame that there's no way to tell prettier to add () around thing. Without this option, there seems to exist no way to achieve error-free formatting like this:

const filteredThings = _.map(things,
  (thing) => thing.x > 0
);

The only option is to refactor, which is not ideal:

const filter = (thing) => thing.x > 0;
const filteredThings = _.map(things, filter);

If things like --tab-width and --trailing-comma are configurable, what's the danger of also having --arrow-parens?

marcinincreo commented 7 years ago

I really appreciate Prettier so I've set "arrow-pens" to 0 in eslint config, but this would be a good addition to prettier to have that configurable.

azz commented 7 years ago

@kachkaev Complete hack, but you could set eslint max-len to 82.

tkirda commented 7 years ago

Except in extreme cases, we are not going to add extra configuration options.

@vramana feels like this is extreme case. Consistency is important and it would be consistent if parens around parameters would always be there.

By adding this option you would make a lot of people much happier and none sadder.

Please consider it.

rattrayalex commented 7 years ago

Reopening this for three reasons:

1) It was the number-one complaint of one team at Stripe which adopted prettier, and is the primary blocker of cross-Stripe adoption. 2) Community interest has been strong (32 upvotes vs 4 downvotes at time of writing, many comments). 3) I personally buy the argument that, with prettier specifically, code is simply best written (x) => x *2.

When you write x => x * 2, prettier can correct you by adding the parens. But when you want to modify x => x * 2 to be x, i => x * i, prettier cannot add the parens for you, leading to a syntax error until you add them yourself.

I intend to submit a PR making this the default behavior shortly.

EDIT: despite the above, I am not confident that this is the best approach, just that it should be reconsidered. arr.map(x => x * 2) is inarguably alluring.

iamstuartwilson commented 7 years ago

Just started using Prettier after a recommendation from a friend at the pub and it's awesome, but I'm also a proponent for an option to force parens and I'll give a couple of reasons why:

1. There's an option for trailing commas. As far as I can see, this options is great because it makes lists easily extendable. Does the same logic not apply to function args?

2. It pleases the brain Having parens for zero or multiple arguments, but not for single args is simply weird when scanning code. It's very easy to recognise a block of code as a function just by the shape of it. Like muscle memory. But the problem is, the deviation from using parens for a single arg all of sudden makes the code not so easy to scan.

3. Not everyone's a pro ES6 (2017, bleh whatever) is a major shift for a lot of people. As a lead in my company, it's nigh on impossible to justify coding standards to other FE devs that just stick out as inconsistent. In my opinion (humble or otherwise) having a consistent codebase which is easy to understand - and compute; see reason two - is a great launchpad for anyone new to ES6 as you spend less time explaining certain weird nuances of the language or adopted style. It's a case of, just because you can (not use parens around arguments) doesn't necessarily mean you should...

โ˜ฎ๏ธ

jlongster commented 7 years ago

Hi, I have not actively been watching prettier issues for a while (sorry about that) but I intend to slowly start reading them again. First off, @kachkaev I highly recommend simply turning off the max-len rule when using prettier. The fact is that the print width is just a recommendation to prettier, and it it can't break something anymore it won't be able to make sure all the code fits within the width. The beauty is that when it doesn't, you don't have to worry about adding a eslint-ignore comment on every single place this happens. Just turn it off and know that 99% of your code will fit. (Think of long strings, and stuff like that that can't be broken up).

I think consistency is still a subjective argument. You can provide examples of how it's inconsistent, but frankly we could show examples of how a lot of styles are inconsistent. The important part is how it's used in real code, and the reason why many people like to drop parens on single args is because short inline functions like x => x.id are so common that it's really nice to drop the parens. The amount of times that I have to go back and add args are few enough that I personally find it a big win.

It could be that if you write heavily functional style code, you are dealing with the above inline functions a lot, so this is a big win for that style of code. But if you aren't doing heavy functional stuff, you may not see this win enough. This is the hard part about formatting.

I'm not sure we should change the behavior. If you agree with me please upvote this comment. There may be an equal number of people on the other side (although they might not see this comment as much as others see the original issue...). We can evaluate if we should add an option. We should really make sure that we can't decide on a single style though.

As said above, the current popular style guides are a strong evidence of the current styles that we need to consider. The airbnb guide enforces no parens on single args, and I know many companies prefer this. This is not a cut-and-dry decision, and we should delay adding options as long as we can (have you tried this style for more than 5 minutes?).

rattrayalex commented 7 years ago

It could be that if you write heavily functional style code, you are dealing with the above inline functions a lot, so this is a big win for that style of code. But if you aren't doing heavy functional stuff, you may not see this win enough. This is the hard part about formatting.

That definitely rings true to me. It sounds like a pretty strong argument in favor of an option in this case. (And certainly an argument against "just make it always have parens", like I suggested).

EDIT: Thanks for chiming in, @jlongster ! miss having you around ๐Ÿ˜‰ And in this case, talking to the team who's been using prettier for a while, it sounds like this remains a sticking point. Also seems to be the case for many on this thread.

vjeux commented 7 years ago

It would be nice to have some real-world examples of where parenthesis look better. So far this thread is full of pseudo-code, which doesn't work well in order to figure out those kind of things.

rattrayalex commented 7 years ago

This may have been mentioned above, but I'd missed it...

The airbnb style guide actually recommends using no-parens only when the arrow-fn has implicit returns:

// bad
[1, 2, 3].map((x) => x * x);

// good
[1, 2, 3].map(x => x * x);

// good
[1, 2, 3].map(number => (
  `A long string with the ${number}. Itโ€™s so long that we donโ€™t want it to take up space on the .map line!`
));

// bad
[1, 2, 3].map(x => {
  const y = x + 1;
  return x * y;
});

// good
[1, 2, 3].map((x) => {
  const y = x + 1;
  return x * y;
});

However, prettier ignores whether the arrow-fn has a block body or an expression body.

Perhaps prettier should adopt airbnb's recommendation and only remove parens when the body is an expression?

EDIT: I agree with @vjeux that more real-world examples are always helpful

vjeux commented 7 years ago

It sounds like a pretty strong argument in favor of an option in this case.

I disagree, there are a lot of codebases with mix of functional and non functional code. Having an option wouldn't really help. What we should first do is to look at examples when people expect parenthesis and when they don't and try to figure out if we can come up with a way to identify those two use cases and put parenthesis in the right place.

kachkaev commented 7 years ago

We have parenthesis in our team's common eslint preset ('arrow-parens': ['error', 'always']). The reasons are similar to what @tkirda @rattrayalex and @iamstuartwilson have recently mentioned.

Tastes differ and that's OK. The today's JS community has some more or less established formatting standards (mostly influenced by airbnb), but some people may still prefer to change --tab-width, --use-tabs, --single-quote and other options just because they want to. If doing so is fine for Prettier, I don't see why --arrow-parens needs to be hardcoded forever as something special. What's the philosophy behind this distinction, given so many +1s on this issue? Who will suffer if --arrow-parens option is added, but remains as is by default?

jlongster commented 7 years ago

@rattrayalex To me airbnb's suggestions sounds even more strangely inconsistent than what we do now. Why does it matter if the body is a block or not? They're probably trying to appease both styles but I think it muddles it even more. I guess it's something to be considered though.

@kachkaev Everyone will suffer in a year after we've added "just one more option" a thousand times. The existing options exist because most of them truly affect 50-100% of code and we are forced to cater to existing styles somewhat for adoption (idealy, we wouldn't have any options). This only affects a small amount of code relative to the whole file and if we start adding options for things of that scale we'll have all of eslint's formatting options.

I'm not saying we should close this issue but there's a reason we don't take adding options lightly. If there's a way to decide on a single style, everybody wins in the long run. It's possible that adding args if braces exist is a compromise we can do.

tkirda commented 7 years ago

I agree with @kachkaev. Remain as is by default so no changes for current users, but allow --arrow-parens option for new users. We have not adopted this across our organization just because of this one feature.

lemonmade commented 7 years ago

I totally appreciate the desire to avoid the "death by a thousand cuts", and so wouldn't be devastated if that meant drawing the line on stuff like this. However, I will say that this is the one thing keep us at Shopify from adopting Prettier more broadly, as it conflicts with our desire (mentioned numerous times above) to keep a consistent paren style for arrow functions (to avoid the no parens -> parens for changing 1 -> >1 args, and to avoid silly things like doing _ => ret to avoid parens for zero param arrow functions). The only argument I haven't seen here is that async arrow functions with parens are (at least, to my eyes), extremely hard to read:

const foo = async bar => baz;
// vs
const foo = async (bar) => baz;
rattrayalex commented 7 years ago

Why does it matter if the body is a block or not?

@jlongster I actually think it's a pretty clever distinction. The primary case where paren-free arrow expressions are awesome are exactly those that you mentioned - when you're chaining simple map/filter/reduce lambdas in that sweet functional style: stuff.map(thing => thing.name). However, anything longer or more complex may suffer in terms of readability by omitting the parens, eg;

getData().then(response => {
  if (response.status === 200) {
    doStuff(response)
  }
})

is (very subjectively) more readable as

getData().then((response) => {
  if (response.status === 200) {
    doStuff(response)
  }
})

However, even if this behavior were much better in most cases, I get the feeling it wouldn't please everybody. I assume that many of the folks on this thread who want the ---no-arrow-parens option would still want that option, as if anything it feels even less consistent, and there are many teams like @kachkaev's who desire consistency in the face of new syntax.

So I see a few paths forward, and personally don't have an opinion yet:

  1. Maintain status quo.
  2. Adopt airbnb's behavior (no-parens for implicit-return arrows only)
  3. Add --no-arrrow-parens option or equivalent.
  4. Both 2 and 3.
  5. An --arrow-parens flag with options always, never, and block (or something like that), perhaps with a block default. (I assume this would be a "hard no" from @jlongster / @vjeux , but mentioning for completeness).

Am I missing anything?

It may be helpful if members of the community could react to ๐Ÿ‘ or ๐Ÿ‘Ž to each of the following comments...

rattrayalex commented 7 years ago

Prefer Option 1: Maintain status quo.

rattrayalex commented 7 years ago

Prefer Option 2: Adopt airbnb's behavior (no-parens for implicit-return arrows only)

rattrayalex commented 7 years ago

Prefer Option 3: Add --no-arrrow-parens option or equivalent.

rattrayalex commented 7 years ago

Prefer Option 4: Both 2 and 3.

rattrayalex commented 7 years ago

Prefer Option 5: An --arrow-parens flag with options always, never, and block (or something like that).

lydell commented 7 years ago

Is option 4 supposed to be "both option 2 and 3"?

TomiS commented 7 years ago

One argument for adding parenthesis for single argument arrow functions would be better compatibility with flow. Let me explain.

Let's say you have a function that is automatically cleaned from having parenthesis:

const func = props => true

Now let's say you want to add flow typings for props and you write this

const func = props: Object => true

This is an error in prettier because it cannot add parenthesis automatically. You need to either realise this upfront or go back and add parenthesis afterwards like this

const func = (props: Object) => true

This is not an end of the world but it definitely doesn't feel like the system is working for you but against you.

Also, I cannot avoid expressing my opinion that I would love the consistency of having parenthesis in all arrow functions instead of 'randomly' skipping the one with single argument.

ps. Prettier is great. This is the only issue that has been continuously annoying me from the start for a few months now.

jlongster commented 7 years ago

@rattrayalex I worry that a poll here is biased because people happy with the current behavior aren't going to be watching this issue.

When it comes to consistency, think of it this way. One "expression" is allowed before the arrow. To capture a single value, you just need a single identifier. To capture multiple values, you need to wrap in parens to capture them, in a way sort of "destructuring". This is not unheard of, and is in fact how OCaml works (except there you literally are destructuring a tuple when getting multiple args).

For type systems, I would think that where arrow functions are used the most (inside functions) that type is already inferred. I use Flow a lot and I can't think of the last time I had to do that, honestly. Doesn't mean the issue doesn't exist though.

When it comes down to it, we're talking about adding parens around an identifier every now and then. Are your companies going to not adopt prettier because of this? Have you gotten your coworkers to use prettier in spite of it? Usually with these kinds of things the wins are so much more than little style disagreements, and people happily accept little disagreements because it's so much better. It's much better for everyone if we try to coalesce on a standard style instead of adding more and more options.

Before you think that I'm just using prettier to force my personal style on other people, there have been many other changes that I disagree with. I hate the if/else syntax, where the else is on the same line as the bracket: } else {. I hate that when objects are expanded, we don't re-collapse them if you remove properties and there's room to re-collapse. We've made changes when there is clearly an established opinion in the community. After prettier has gotten more and more adoption from large libraries without complaints (react, babel, etc), we've resisted changes more in order to reduce the style variations. A very large number of people are happy with the current style. That doesn't mean we won't change anything, but at some point we should just lay our own style preferences to rest and just do whatever prettier does. We aren't going to add as many options as elint's formatting rules, and I'm not going to maintain prettier forever.

lydell commented 7 years ago

Just for correctness sake I have to point out that parentheses are required even around single-argument arrow functions when you use destructuring or default values.

foo => foo // ok
{foo} => foo // not ok
({foo}) => foo // ok
foo = 5 => foo // not ok
(foo = 5) => foo // ok
rattrayalex commented 7 years ago

Hmm, well, I'm not really sure where this leaves us... I'll try to put together a PR soon adoption Option 2, adopting Airbnb's style of adding parens unless the function body is an implicitly-returned expression (though I'm not sure the PR should be accepted). I saw a ๐Ÿ‘ reaction from @jlongster , though of course that's not necessarily an endorsement. The "poll" feels somewhat inconclusive, especially due to the selection bias mentioned by @jlongster .

When it comes down to it, we're talking about adding parens around an identifier every now and then. Are your companies going to not adopt prettier because of this?

As crazy as it seems, the answer appears to be "yes" ๐Ÿ˜„ I think the fact that so many people have found this to be their number-1 complaint about prettier is a massively positive indication about prettiers quality, and speaks volumes to the thoughtful work of maintainers/contributors thus far.

So hopefully we can come up with something great here too.

azz commented 7 years ago

Looking around some real code, currying hasn't really been mentioned.

Code as-found:

const line = pkg => dep => `"${name(dep)}" -> "${name(pkg)}";`;

Code with parens:

const line = (pkg) => (dep) => `"${name(dep)}" -> "${name(pkg)}";`;

The Airbnb suggestion is starting to sound like the winner here.

rattrayalex commented 7 years ago

Opened a PR at https://github.com/prettier/prettier/pull/2430, feel free to comment there.

bakkot commented 7 years ago

FWIW, the proposed change really feels like a regression to me. The inconsistency between a => a == null ? 0 : 1 and (a) => { if (a == null) return 0; else return 1; } is especially annoying. Someone who has read AirBnB's rules might understand where the difference coming from, but to someone just reading the code the inconsistency is glaring.

Also... this would affect a lot of code. Prettier has been at 1.0 for a while, and this sort of major change is really disruptive to people using the project. Just by way of illustration, prettier's printer.js has 73 arrows, of which 60 have a single identifier as their parameter list, and 24 of those 60 have function bodies. So to this one file this change would add 24 unnecessary sets of parentheses.

By comparison, there are only ~18~ (edit: 24) changes between formatting that file with 1.0.0 vs with master. This would ~more than~ double that number.

rattrayalex commented 7 years ago

Those are certainly helpful stats!

azz commented 7 years ago

There is one option that hasn't been mentioned in this issue: respecting the source.

Pros :+1: Cons :-1:
No churn in the upgrade process Can lead to inconsistent code
No need to maintain an option ?
Can satisfy all styles (as-needed, block, always) ?
jlongster commented 7 years ago

I thought about that option too, but it seems too inconsistent with our message of having a consistent style everywhere.

@rattrayalex Yeah, it's great that we're at the point where this is one of the hottest debates. My concern is that it won't stop though; we'll change this but whatever isn't the 2nd most request change will then come to the top. At some point we're going to have to say that prettier is "frozen" except for new JS language features that we need to implement. We're not going to make everyone happy, but I'm willing to bet in the long run it's the right choice to minimize options and freeze our style. The question is when we should freeze it (I think there are several more things to look into before that).

In terms of breaking compatibility, I think it's OK every now and then. The whole point of prettier is that it doesn't actually effect anything except the git history; upgrade and everything will instantly be formatted with the new style. We should be careful not to make too many changes just to avoid the churn, but every now and then it's OK.

jlongster commented 7 years ago

I'm not able to really focus on this right now but I'll help us make a decision soon by involving @vjeux and asking more people outside this issue which style they prefer.