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

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

Closed
DavidBabel opened this issue Feb 25, 2017 · 103 comments
Labels
lang:javascript Issues affecting JS status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Milestone

Comments

@DavidBabel
Copy link

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) => {}
@vramana vramana added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Feb 25, 2017
@andreypopp
Copy link

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
Copy link
Author

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
Copy link

vramana commented Feb 27, 2017

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 => ...).

#572 (comment)

Therefore I am closing this issue.

@vramana vramana closed this as completed Feb 27, 2017
@jlongster
Copy link
Member

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
Copy link
Contributor

@DavidBabel I just added this option in arijs@0f91846

@DavidBabel
Copy link
Author

@rhengles awesome

@androa
Copy link

androa commented Apr 6, 2017

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 😜

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. #812 (comment)), or is this something worth finishing and PR?

@drewhamlett
Copy link

drewhamlett commented Apr 23, 2017

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
Copy link

dejanr commented Apr 27, 2017

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
Copy link

rmoorman commented May 1, 2017

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
Copy link

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
Copy link

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
Copy link

androa commented May 3, 2017

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
Copy link

drewhamlett commented May 3, 2017

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
Copy link

burabure commented May 19, 2017

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

@lydell lydell changed the title Add support for arrow-parens property Parentheses around single parameter arrow functions / Add support for arrow-parens property May 25, 2017
@dlindenkreuz
Copy link

dlindenkreuz commented Jun 3, 2017

@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
Copy link

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
Copy link
Member

kachkaev commented Jun 7, 2017

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
Copy link

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
Copy link
Member

azz commented Jun 12, 2017

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

@tkirda
Copy link

tkirda commented Jul 6, 2017

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
Copy link
Collaborator

rattrayalex commented Jul 6, 2017

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.

@rattrayalex rattrayalex reopened this Jul 6, 2017
@iamstuartwilson
Copy link

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
Copy link
Member

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?).

@bakkot
Copy link
Collaborator

bakkot commented Jul 23, 2017

In modern JavaScript, where => is often encouraged over function, that's extremely common code. I haven't seen anyone claim they really want no-parens in that situation.

Allow me to make that claim, then. Furthermore, even if I were neutral on the question I would strongly prefer prettier not make such large changes to its style, at this point.

@paldepind
Copy link

@rattrayalex

There are some straightforward heuristics we can use here. If it's a curried function "declaration", don't use parens. If it's a callback, don't use parens. Otherwise, always use parens.

From my point of view that is worse than the current behavior. The reason why I prefer to always add parens is that I don't like the extra complexity and inconsistency that the edge case in the syntax adds. From what I can tell that also seems to be why other people in this thread prefer to always add parens.

Leaving out the parens has the small benefit of saving two characters but on the downside reduces consistency and makes refactoring more work. That doesn't, in my opinion, justify adding an edge case and thus complexity to the syntax. Therefore I prefer to write my JS as if that edge case didn't exist.

The Airbnb style guide and what you're proposing only adds extra rules and more complexity. It makes the edge case even more "edgy". I definetly prefer the current behavior to that.

@plee-nm
Copy link

plee-nm commented Jul 25, 2017

@rattrayalex

How simple would it be just to have a config option that doesn't do anything with single parameter arrow functions? Let the developer decide how it should be if they want it off. If they have it on just keep the current behavior. Does this sound good or does it go against the philosophy of prettier?

@ariesshrimp
Copy link

ariesshrimp commented Jul 25, 2017

@plee-nm the team has said several times in this thread that

  1. it would not be hard
  2. they will not allow it

the point of using prettier is to give up your opinions about code aesthetics. there is no non-arbitrary reason to make the arrow-parens option configurable, but not other options. if all options are configurable, this is just eslint

@plee-nm
Copy link

plee-nm commented Jul 25, 2017

@joefraley Sorry I really haven't read through the thread. I guess to me this is a gray area where prettier is changing my code to be slightly less maintainable and a 50-50 split on what is the right way like single vs double quotes and tabs vs spaces.

@ariesshrimp
Copy link

ariesshrimp commented Jul 25, 2017

@plee-nm no apology is necessary, i only meant to summarize what had been said so far (it's a long thread 😉)

@michaljuris
Copy link

The option to allow parens should exist at least for the Typescript, because TSLint enforces parens for a single parameter by default.

@azz
Copy link
Member

azz commented Jul 26, 2017

tslint-config-prettier will turn off the arrow-parens rule.

@niftylettuce
Copy link

niftylettuce commented Aug 17, 2017

This is still open? Why not add an option? I appreciate #812 (comment) but relying on a fork (that may go unmaintained or not synced to upstream) might not be the best approach. It is unfortunate I cannot use https://github.com/prettier/vim-prettier with this due to a single option.

@bakkot
Copy link
Collaborator

bakkot commented Aug 17, 2017

@niftylettuce

Why not add an option?

A lot of people, my self included, would strongly prefer prettier resist adding configuration. See also @jlongster on this particular question in this thread and elsewhere.

@TomiS
Copy link

TomiS commented Aug 17, 2017

As someone who isn't affiliated with prettier at all and have used it for some months now, I'd like to say to everyone who resists using prettier because of this issue. Please, do yourself a favour and start using it anyway. Or try it at least. This ticket is about fine-tuning some edge-cases to move prettier from 95% perfectness to 96% and no one should not use it because of this. The thing is, with prettier you can get rid of a major cognitive load that comes from caring how the code looks like. You just accept that prettier is correct and focus on more important things. That's 100 times more impactful to developer performance than any minor detail in style can ever be.

ps. Sorry for off-topic, but I felt like this needed to be said (again).

@michaljuris
Copy link

@niftylettuce @jlongster I strongly disagree on that. It is really quirky to have rely on a fork when I want to use prettier with Typescript.
Pls consider add that option or change default for Typescript.

@niftylettuce
Copy link

niftylettuce commented Aug 17, 2017 via email

@w0rp
Copy link

w0rp commented Aug 18, 2017

I would also like to have prettier work so it will leave existing arrow functions how they are. I would like the following code to be left alone.

const f = (x) => 1
const g = x => 2

In my codebase, there's no preference for either style. Either form is allowed, because there's no semantic difference. The trouble with changing the code to use either style is that it results in larger diffs for code, and if you change lines that don't need to be changed, then you get merge conflicts.

@idrougge
Copy link

I think you misunderstand what Prettier is. Prettier is an opinionated code formatter. It changes your code. By using Prettier, you give up your own style of code.

@w0rp
Copy link

w0rp commented Aug 18, 2017

Then users will give up using prettier.

@idrougge
Copy link

Of course they will. Because what they want is ESlint, not Prettier.

@burabure
Copy link

It would seem to me that this thread has run it's course and will produce no useful conversation anymore; at this point I'd be happy with whatever solution just so that people would stop turning this thread into a cesspool.

Maybe it's time to close it for good and let someone else open a new issue if they want to?

@w0rp
Copy link

w0rp commented Aug 18, 2017

I wouldn't call discussion about legitimate concerns for configuring software to make it suitable for use in existing projects a "cesspool."

@rattrayalex
Copy link
Collaborator

rattrayalex commented Aug 19, 2017

This is the most-commented thread on the prettier repo.

There isn't much that could be said on this topic that hasn't been said at least once on this thread.

I hesitate to close this issue since it is clearly a sticking point for many users. For example, it's received a large number of upvotes (it's in the top 10 of all prettier issues, and the informal poll I posted above has garnered notable activity over a sustained period of time). I think we can do better than the current behavior and I believe in the prettier team's ability to come up with something good.

I am leaving this issue open but locking conversation until another maintainer is able to take a look and make a definitive decision or re-open the conversation.

@prettier prettier locked and limited conversation to collaborators Aug 19, 2017
@azz azz added lang:javascript Issues affecting JS status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity. and removed status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Oct 4, 2017
@azz azz added this to the Airbnb milestone Nov 7, 2017
@bakkot
Copy link
Collaborator

bakkot commented Dec 1, 2017

This was resolved by #3324.

@bakkot bakkot closed this as completed Dec 1, 2017
@azz azz modified the milestones: Airbnb Compat, 1.9 Dec 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:javascript Issues affecting JS status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Projects
None yet
Development

No branches or pull requests