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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Space after function name in declarations #3845

Open
justrhysism opened this Issue Jan 31, 2018 · 164 comments

Comments

Projects
None yet
@justrhysism
Copy link

justrhysism commented Jan 31, 2018

NOTE: This issue is raised due to confusion in #1139.
NOTE: Feel free to use the 馃憤 and 馃憥 buttons to indicate your preferences, but please refrain from commenting unless you鈥檝e read every comment and made sure yours says something that has not been said before.

This issue is only about outputting these spaces:

function identity (value) {
  //             ^ space here
  return value;
}

function identity <T>(value: T): T {
  //             ^ space here
  return value;
}

class A {
  method () {
    //  ^ space here
  }
}

const obj = {
  method () {
    //  ^ space here
  }
}

(Prettier currently does not put spaces in those positions.)

馃棐 NOTE: This issue is not about anonymous functions:

const identity = function (value) {
  //                     ^ space here: SEE #3847!
  return value;
}

Anonymous functions are tracked in #3847!

The key arguments is:
Searchability: You can find any function or method declaration my searching for e.g. method ( or identity <, or even just name[space] (mostly).

It also provides a clear and immediate difference between declaration and invocation.

(There are many existing comments in favour of this style, notably: #1139 (comment) and the majority of the discussion from #1139 (comment) onwards.)

@brianjorden

This comment has been minimized.

Copy link

brianjorden commented Jan 31, 2018

This is one of the reasons I've had to step away from adopting Prettier. I would be quite happy with just an optional flag. The original issue talked a lot about configuring and adding in eslint to re-process and alter the output of the code AFTER prettier has a go at it. My understanding from what I read is that was being suggested to "reduce configuration" that people generally needed to do and obviously it is just trading where configuration is being done.

Additionally, many people don't actually use ESLint, so the suggestion is to actually bolt on another entire tool to the build chain. That falls down further when talking about a combination of TypeScript + Vue + Vetur + Single File Components as right now I don't believe there is a working implementation to combine TSLint + Prettier + Vetur.

I currently maintain a fork of Prettier mainly for this reason, but it breaks constantly. An additional challenge comes in with Vetur not currently using the locally installed version of Prettier, but the Prettier VSCode extension doesn't run on .vue files. I've resorted to manually overwriting the installed version of prettier in three node_modules locations every time things get updated (local project, global, and vetur). Needless to say, it breaks a lot.

Many people/companies/projects can't simply change certain aspects of their code spec requirements on a whim. Prettier matches, or is close enough to the vast majority of rules we're using, and obviously the rules in some of the most popular sets of standards used with things like ESLint, except this ONE item. Most other variations can be dealt with through a prettier-ignore comment, but obviously this is extremely wide spread and a major point of contention.

I hope that all those involved will look at and consider the rather large number and volume of voices just asking to incorporate an opt-in ability to handle adding a simple space. The arguments of this being "bike shedding" feel a bit silly to me. The alternatives are to either disable rules within the most popular sets of linting rules OR add an entire extra set of tooling/parsing of every file.

@j-f1

This comment has been minimized.

Copy link
Member

j-f1 commented Jan 31, 2018

@brianjorden

This comment has been minimized.

Copy link

brianjorden commented Feb 1, 2018

I get all of that, but on that same logic, why would the core and ONLY option of Prettier then violate the extremely popular much older linting/coding standards on this exact issue? I totally get doing things in a very opinionated way, but the standard and airbnb rules have been downloaded millions of times per month since long before Prettier existed. Honestly, I personally don't even like a lot of those "out of the box" rules. The oddity to me is that this is the ONE item that seems to violate the prior defacto-standard sets of rules that a lot of people have been using for a long time. I may be wrong in my overall understanding, but if the concept is to avoid the "holy wars" and "bike shedding" cliche concepts, why is this one rule different than some of the popular standards?

@lydell

This comment has been minimized.

Copy link
Collaborator

lydell commented Feb 1, 2018

First off, please note that we're not stubbornly saying no to this. This issue is open, and is now part of the process for adding options. That process isn't formally written down (like ESLint's) but it has kind of started to grow out of itself.

We're now waiting for that 馃憤 counter to increase. Give it some time.

The original issue talked a lot about configuring and adding in eslint to re-process and alter the output of the code AFTER prettier has a go at it. My understanding from what I read is that was being suggested to "reduce configuration" that people generally needed to do and obviously it is just trading where configuration is being done.

Additionally, many people don't actually use ESLint, so the suggestion is to actually bolt on another entire tool to the build chain.

The suggestion is generally to let it go and just use Prettier. But if you can't do that, there's always the option of using prettier-eslint. Or not using Prettier at all. Or wait, and see if people change their minds.

except this ONE item

Everyone says that in every issue :) "This is the ONLY thing preventing us from using Prettier." We'd have a lot of fix-somebody's-only-problem options at this point if that was a convincing argument :)

Many people/companies/projects can't simply change certain aspects of their code spec requirements on a whim.

This might be a silly, but this is how I imagine that scene playing out:

employee: I've found a tool that could reduce the time we spend on formatting code and fixing lint issues. Can we give it a try?
boss: Does it comply with our style guide?
employee: Yes! Except one rule.
boss: Sorry, our arbitrary choices are more important than the productivity of our employees.
employee: Can we at least give it a try to evaluate it?
boss: No! Those spaces are part of the company's core values.

I mean if Prettier complies with all of your style guide except one item, isn't that a huge success? If you can't even change one item of your style guide 鈥 that's even about just a single space 鈥 I'm not sure if you should use Prettier at all. Every new Prettier version runs the risk of inroducing some minor conflict with your style guide.

Honestly, I personally don't even like a lot of those "out of the box" rules. The oddity to me is that this is the ONE item that seems to violate the prior defacto-standard sets of rules that a lot of people have been using for a long time. ... why is this one rule different than some of the popular standards?

Because it's not that easy. In fact, I've very rarely worked on a code base that use the space-after-function-name rule. Hadn't even heard of it before I found standard. Remember, when James Long first implemented the core printing he had a lot of decisions to make. Perhaps he hadn't worked on many code bases with space-after-function-name either and didn't think about it. Or he just needed to choose something to be able to move on, so Prettier could be released and not stuck in decision making forever. We try hard to choose non-controversial style decisions (for markdown we even used bigquery to gather stats), but we also have to consider the churn of making changes that affect all existing Prettier users. So sometimes we're stuck with decisions made in the past, and it takes time to find the best way forward. So this is not about the Prettier authors trying to impose their style preferences to the world. It's about historic decisions, which plague all software and specifications.

It is also not a clear "defacto standard". ljharb from airbnb says (#3503 (comment)):

the most common and consistent standard for function keyword spacing in the ecosystem is "function keyword", "space", "optional name", "argument list".

I hope that all those involved will look at and consider the rather large number and volume of voices

We certainly will! Consider the fact that the original issue was split and re-opened through this one a good sign. Now, we need to let things settle a little. Give it a think-through. Gather some more 馃憤s and comments.

(Or perhaps somebody makes a PR that is merged next week. Who knows.)

@bakkot

This comment has been minimized.

Copy link
Collaborator

bakkot commented Feb 1, 2018

Neither AirBnB nor Google recommend this style. It is not the style used by jquery, react, preact, bootstrap, babel, TypeScript, flow, express, angular, v8, spidermonkey, RxJS, lodash, istanbul, node, ember, d3, socket.io, qunit, jest, ava, jasmine, webpack, polymer, chartjs, gulp, bluebird, ramda, esprima, yarn, handlebars, immutable, uglifyjs, grunt, jsdom, glamorous, koa, eslint, or indeed prettier itself.

Not to say prettier's style is universal; there are exceptions: browserify, moment (sometimes), mocha, rollup, vue, npm. But these are much, much rarer.

I was going to say something about how the style suggested in this issue wasn't the de facto standard in the ecosystem, but now that I've gone and looked, I'm going to make a stronger claim:

There is in fact an overwhelming de facto standard in the ecosystem, and it is what prettier already does.

I don't think we should worry too much about votes. The ecosystem is far larger than this issue tracker could possibly reach. This seems like a place prettier should stick to its guns.

Edit, 2018-05-11: both mocha and rollup have adopted prettier and the no-space style; I've struck them out from the list of non-examples.

@j-f1 j-f1 referenced this issue Feb 1, 2018

Open

Prettier 2.0 #3503

@brianjorden

This comment has been minimized.

Copy link

brianjorden commented Feb 1, 2018

First off, I was completely wrong on the AirBnB thing (and just wasn't sure on the Google suggestions). Second, although I don't know that I should go back and edit my prior comments, they did likely come off way too harsh/judgy/stubborn on my part, and sorry for that.

As to the usage and frequency in the ecosystem, there are some fairly sizable projects/groups that rely on standard and therefore likely are following this rule. (I had put together a little list before I saw bakkot's above.) Things like NPM, GitHub, Electron, Atom, Moment, Vue, Zeit, MongoDB, Zendesk, Mocha, Karma, WS, and https://raw.githubusercontent.com/standard/standard-packages/master/all.json

I'll admit, by no means a consensus or majority, but some non-trivial sized groups involved there. Also, the idea of "how that conversation would go" thing is amusing, but doesn't quite fly in the consulting world when talking about deliverables, contracts that call for code spec conformance, and a multi-layered IT department of even a moderately sized corporation. The suggestion of "just use eslint" doesn't quite work when I had explained that isn't actually an option (TypeScript + Vue + Vetur).

You also mentioned that this isn't just people being stubborn, and although of course the maintainers/original creator has every right to be stubborn, it does feel like there is a bit of that honestly. Couple small examples:

In reading through some of the 2.0 comments, it has become pretty clear I need to back off and find a new approach though. I know it got retracted a bit, but even comments about possibly removing TypeScript support came up over there. I do like and appreciate what this group has contributed and look to be contributing in the future. I'd love to actually do the PR to add the config option for this, but that proposal got completely shot down in the other threads. Just seems like sometimes groups (not just this one) can be a bit uninviting to new contributors or even discussion about a possible contribution.

My only point in trying to put so much effort into showing that there aren't realistic options to "just use another tool as well" and that a decent number of people/groups/projects would benefit from this, is that I think the "bike shedding, closing issue" thing can be a bit misused at times. To me and I think to many others, it is more "my bike has 3 wheels and can't fit through the door, could we make the door slightly wider?" and the response is "most bikes fit, go away".

@lydell

This comment has been minimized.

Copy link
Collaborator

lydell commented Feb 1, 2018

@brianjorden Don't worry, this issue is open and isn't going anywhere! Just be patient :) (The original issue was closed when Prettier was just a couple of months old and we didn't know how to handle Prettier's popularity growth yet.)

even comments about possibly removing TypeScript support came up over there [in the 2.0 discussion]

You're misunderstanding 鈥 TypeScript support will never be removed. If anything it could be moved outside the Prettier core, but I doubt that.

@j-f1

This comment has been minimized.

Copy link
Member

j-f1 commented Feb 1, 2018

As one of the people advocating the removal of the TypeScript parser, I鈥檇 like to clarify: Babylon 7, our JS parser, has support for TypeScript. However, we currently use TypeScript鈥檚 parser to parse TypeScript. I was suggesting moving the code that uses TypeScript鈥檚 parser to an external plugin, then use Babylon to parse the TypeScript, decreasing the bundle size.

@bakkot

This comment has been minimized.

Copy link
Collaborator

bakkot commented Feb 1, 2018

Things like NPM, GitHub, Electron, Atom, Moment, Vue, Zeit, MongoDB, Zendesk, Mocha, Karma, WS, and https://raw.githubusercontent.com/standard/standard-packages/master/all.json

Interestingly enough, GitHub, MongoDB, and Zeit are already using prettier (elsewhere?). Mocha has been discussing adopting it, and a couple of the owners specifically mention not caring about this particular issue. So I don't think the lack of an option here is all that much of a barrier in general, even though it might be on some specific projects.

@TrySound

This comment has been minimized.

Copy link

TrySound commented Feb 1, 2018

Btw rollup is also gonna adopt prettier, so its own code style will be eliminated
rollup/rollup#1896

@justrhysism

This comment has been minimized.

Copy link
Author

justrhysism commented Feb 1, 2018

a couple of the owners specifically mention not caring about this particular issue

Those that don't care, by definition, wouldn't care if it was implemented.

@j-f1

This comment has been minimized.

Copy link
Member

j-f1 commented Feb 2, 2018

Those that don't care, by definition, wouldn't care if it was implemented.

Apathy isn鈥檛 a good reason to add a new feature :)

@justrhysism

This comment has been minimized.

Copy link
Author

justrhysism commented Feb 5, 2018

Apathy isn鈥檛 a good reason to add a new feature :)

Agreed. With respect to adding weight to an argument, apathy is neither for, nor against an argument - which is the point I was trying to make.

@Ahimta

This comment has been minimized.

Copy link

Ahimta commented Feb 20, 2018

I need to have spaces after function because this conflicts with Standard and I have to run a lint script manually that runs Prettier then Standard. Because I can't have save-on-format in my editor to do the same thing.

I don't disagree with Prettier not having spaces after function, it's just very inconvenient for me.

Thanks and keep up the good work!

@j-f1

This comment has been minimized.

Copy link
Member

j-f1 commented Feb 20, 2018

@Ahimta Have you seen prettier-standard?

@duailibe

This comment has been minimized.

Copy link
Member

duailibe commented Feb 20, 2018

Agreed. With respect to adding weight to an argument, apathy is neither for, nor against an argument - which is the point I was trying to make.

In this case, apathy is an argument against implementing this.. if most people don't care about it, there's no reason for us to carry the burden of supporting in the future.

@Ahimta

This comment has been minimized.

Copy link

Ahimta commented Feb 26, 2018

@j-f1 what I really want is editor support. Otherwise, I could easily use git hooks or watch scripts -_-.
If Prettier decides to add an option for a space after function I may have enough motivation to implement it myself :).

@lydell

This comment has been minimized.

Copy link
Collaborator

lydell commented Feb 26, 2018

@Ahimta It sounds like you have confused this issue with #3847.

@Ahimta

This comment has been minimized.

Copy link

Ahimta commented Feb 26, 2018

@lydell sorry I mean both (all function declarations). Since this is the one thing that Standard doesn't allow.

@lydell

This comment has been minimized.

Copy link
Collaborator

lydell commented Feb 26, 2018

sorry I mean both (all function declarations).

Whitespace is mandatory between the function keyword and the function name. This issue is not about a space after function. It is about a space after the function name. See the OP.

@Ahimta

This comment has been minimized.

Copy link

Ahimta commented Feb 26, 2018

@lydell I mean exactly these two cases:

  1. function () { }
  2. function f () { }
@RomainLanz

This comment has been minimized.

Copy link

RomainLanz commented Oct 29, 2018

Any news on this?

@phlogisticfugu

This comment was marked as off-topic.

Copy link

phlogisticfugu commented Dec 7, 2018

I just spent a few hours drilling through error messages and issues to finally get here. This is a continuing issue for many people. In the Vue world, most of the out of the box configurations I use (nuxt, vue-cli) are associated with the standard eslint rules.

Could we perhaps update the prettier documentation at https://prettier.io/docs/en/eslint.html to mention something along the lines of:

"Compatibility Notes: Prettier is a highly opinionated formatter, and is known to have differences with the default settings of several popular eslint configurations including:"

https://github.com/standard/eslint-config-standard
https://github.com/vuejs/eslint-plugin-vue

... etc

and, if someone could please also document on that same page some steps one can take to resolve such issues. It's not entirely clear for someone who doesnt normally monkey about in eslint what does what on that page.

@tmorehouse

This comment has been minimized.

Copy link

tmorehouse commented Jan 12, 2019

It appears adding in an option to allow the space beween method name and opening parenthesis is gaining more plusses:

image

I definitely prefer having the space in there when defining methods (and no space when calling methods).

@avrelaun

This comment has been minimized.

Copy link

avrelaun commented Jan 18, 2019

This and #3847 are the only things missing for relying on prettier alone for style on all our company projects...

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jan 18, 2019

/cc @prettier/core What is your opinion now? Based on count 馃憤 and 馃憥 we should add new option here and move ahead.

@suchipi

This comment has been minimized.

Copy link
Member

suchipi commented Jan 18, 2019

Indifferent, but I agree that if we add it it should be an option

@duailibe

This comment has been minimized.

Copy link
Member

duailibe commented Jan 18, 2019

I don't think we should change it. I'm always 馃憥 for adding options.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jan 18, 2019

@duailibe not change, just add new option based on 馃憤 and 馃憥 , a lot of developers want this, i also don't love new option, but here it looks like holy war option.

@bakkot

This comment has been minimized.

Copy link
Collaborator

bakkot commented Jan 18, 2019

This is very far from being a holy war. Tabs vs spaces was a holy war. Semicolons was a holy war. This is not.

@lipis

This comment has been minimized.

Copy link
Member

lipis commented Jan 18, 2019

My vote is to change it and make a release with broken changes.. Having space is better and makes sense, and there is no need for adding new option!

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jan 18, 2019

@lipis hm, for me space here is very ugly 馃槃

@duailibe

This comment has been minimized.

Copy link
Member

duailibe commented Jan 18, 2019

@evilebottnawi I don't think this is holy war.

I don't think this change justifies a new option. But that's me..

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Jan 18, 2019

/cc @vjeux we need your opinion here 馃槃

@vjeux

This comment has been minimized.

Copy link
Collaborator

vjeux commented Jan 22, 2019

Looking at the top issues, the first two are about space after functions. If we're going to add a new option, this is the one we should add. I'm not against it given how many people seem to want it.

image

@stevefan1999-personal

This comment was marked as off-topic.

Copy link

stevefan1999-personal commented Jan 22, 2019

This 'feature'/misbehavior really irritate me on using TSLint, StandardStyle and Prettier.
It has been YEARS and people is still arguing about it. Just add it as an option that is disabled by default in a new config key called compat

@bakkot

This comment has been minimized.

Copy link
Collaborator

bakkot commented Jan 22, 2019

There will always be a top issue. If prettier adds an option for this, something else will be the top issue. I really think it is important to stick to the options philosophy and resist adding configuration, especially when there is an overwhelming de-facto standard in the ecosystem which matches what prettier currently does.

@aprilmintacpineda

This comment has been minimized.

Copy link

aprilmintacpineda commented Jan 23, 2019

Here's what you can do guys:

npm run prettier --write && eslint --fix

Run in in that order and you get eslint to fix what prettier kind-of messed up. This is assuming you are using both.

@duailibe

This comment has been minimized.

Copy link
Member

duailibe commented Jan 23, 2019

@aprilmintacpineda There's a tool that combines both: https://github.com/prettier/prettier-eslint you should check if it suits your requirements.

@avrelaun

This comment has been minimized.

Copy link

avrelaun commented Jan 23, 2019

@aprilmintacpineda We already do that at Beekast, but this is definitely not optimal: Almost all our codebase files are rewritten by prettier on the filesystem then again by eslint. This is slow and painful for the storage.

In order to ditch eslint for style we can ignore pretty much all our eslint rules without much compromise except this for this one and #3847

Relying on eslint for errors only and prettier for style does makes sense for us.

@coolemur

This comment has been minimized.

Copy link

coolemur commented Jan 23, 2019

screenshot 2019-01-23 at 15 23 04

One year later.

馃憤 counter approves the change.

Prettier team still debates if they should or should not change it.
And they still looking for other reasons not to implement the option.

I just don't get it, why you keep this discussion open if you already have an opinionated decision for it. 馃憥

@lydell

This comment has been minimized.

Copy link
Collaborator

lydell commented Jan 23, 2019

Unlike other issues with lots of 馃憤, this one has lots of 馃憥 too.

@coolemur

This comment has been minimized.

Copy link

coolemur commented Jan 23, 2019

Unlike other issues with lots of 馃憤, this one has lots of 馃憥 too.

Thats just an excuse anyway.
馃憤 > 馃憥.

@gillesdemey

This comment has been minimized.

Copy link

gillesdemey commented Jan 23, 2019

Those that have voted 馃憥 would still be comfortable because the rule would be opt-in.

@josefaidt

This comment has been minimized.

Copy link

josefaidt commented Jan 23, 2019

would you say the spread warrants an option addition rather than including it in the default spec without the ability to opt-out?

@coolemur

This comment has been minimized.

Copy link

coolemur commented Jan 23, 2019

would you say the spread warrants an option addition rather than including it in the default spec without the ability to opt-out?

Without option those who voted 馃憥 would have the same issue that those who voted 馃憤 have currently.

@duailibe

This comment has been minimized.

Copy link
Member

duailibe commented Jan 23, 2019

I just don't get it, why you keep this discussion open if you already have an opinionated decision for it.

@coolemur Why does that bother you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment