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

Always add a space after the function keyword #3903

Merged
merged 9 commits into from Jan 20, 2020

Conversation

j-f1
Copy link
Member

@j-f1 j-f1 commented Feb 6, 2018

Fixes #3847.
Closes #7028.

Seems pretty simple to do, so I tested it out to see how it looks. If you want to test this out on your codebase, you can run:

$ yarn add --dev j-f1/prettier#space-after-function

There is also the playground for testing things out.

@j-f1 j-f1 added status:needs discussion Issues needing discussion and a decision to be made before action can be taken status:wip Pull requests that are a work in progress and should not be merged labels Feb 6, 2018
@lydell
Copy link
Member

lydell commented Feb 6, 2018

Could you update the snapshots?

@josephfrazier
Copy link
Collaborator

I'm excited to see this moving along, so I updated the snapshots, then fixed generator functions per #3847 (comment). @j-f1, if that wasn't ok with you, my apologies, and feel free to force-push changes as desired.

@j-f1
Copy link
Member Author

j-f1 commented Feb 6, 2018

Go ahead @josephfrazier. I’m not at a 🖥 right now, so your help is appreciated 👏

@josephfrazier
Copy link
Collaborator

josephfrazier commented Feb 6, 2018

The test failure is happening on other PRs, and appears to be related to #3875

EDIT: Looks like this was resolved, or at least retried until it worked!

@j-f1 j-f1 removed the status:wip Pull requests that are a work in progress and should not be merged label Feb 21, 2018
@j-f1 j-f1 changed the title WIP/RFC: Always add a space after the function keyword RFC: Always add a space after the function keyword Feb 21, 2018
@vjeux
Copy link
Contributor

vjeux commented Feb 21, 2018

This is a major change which will affect almost every single file in people codebases. If we ship this, we should do it behind an option. Since this is controversial, we don't want people to have to fight with their colleagues for doing an upgrade and possibly not upgrade because of it.

@lydell
Copy link
Member

lydell commented Feb 21, 2018

This is a major change which will affect almost every single file in people codebases.

Really? I thought people mostly used arrow functions instead of anonymous "regular" functions these days.

In the code bases I work on, the only places function () {} is used instead of () => {} are jQuery callback where we need jQuery's this, which are a minority of all callbacks.

@vjeux
Copy link
Contributor

vjeux commented Feb 21, 2018

I just did a query for function() on our js codebase and there were too many results to display. There's a bunch of existing code and not everyone is using arrow functions everywhere. If you look at the snapshot tests, more than 80 changed, so it's going to affect a ton of people.

@lydell
Copy link
Member

lydell commented Feb 21, 2018

I see. If so, I'd rather close this than add an option.

@j-f1
Copy link
Member Author

j-f1 commented Apr 13, 2018

The accompanying issue now has (98-16=) 82 👍s, so it seems like there is significant demand for this. Should we create a 2.0 branch and start merging breaking changes like this one into it?

@suchipi
Copy link
Member

suchipi commented Apr 13, 2018

I don't think we should do this, even as a breaking change in 2.0. Maybe an option, but I don't like adding more things for people to make decisions about; defeats one of prettier's strong points.

@j-f1
Copy link
Member Author

j-f1 commented May 26, 2018

Closing this for now to clean up the PR list, but I won’t delete the branch and I’ll reopen if we agree to do this.

@j-f1 j-f1 closed this May 26, 2018
@raszi
Copy link

raszi commented Jun 15, 2018

136 👍 - 20 👎 + 20 ❤️ = 136 upvotes

If you decided to add the --arrow-parens option because of the "huge demand" (50 upvotes), then you should please reconsider this.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Sep 13, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 13, 2018
@j-f1 j-f1 added keep-unlocked and removed locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. labels Oct 4, 2018
@prettier prettier unlocked this conversation Oct 4, 2018
@m1ck2
Copy link

m1ck2 commented Oct 4, 2018

Can I join in here

@j-f1
Copy link
Member Author

j-f1 commented Oct 4, 2018

This is finished and ready to go @m1ck2. All I need to do is fix conflicts when the team decides to start working on 2.0.

@j-f1 j-f1 mentioned this pull request Oct 26, 2018
@kirillgroshkov
Copy link

Please ❤️

@bovas85
Copy link

bovas85 commented Oct 30, 2018

I avoided all this adding eslint and using prettier reading eslint

@thorn0 thorn0 merged commit b47f7ba into prettier:next Jan 20, 2020
@glen-84
Copy link

glen-84 commented Jan 31, 2020

Why was the formatting of generators changed based on an issue with 2 up-votes?

Shouldn't such changes be subject to additional consensus?

Both MDN and the Airbnb JavaScript Style Guide use the previous formatting, FWIW.

Please consider reverting this part of the change, and reopening #7028.

@brainkim
Copy link
Contributor

brainkim commented Feb 1, 2020

@glen-84 Note that the previous guideline in the AirBnB guide states:

11.2 Don’t use generators for now.

If you have any objections besides precedent (which I address in the issue), post a comment in #7028. I will be waiting ⚔️🛡. My suspicion is that most people don’t know or care about generator stars enough to have an opinion on them. I, on the other hand, am literally building a framework around generators, so there’s that.

@glen-84
Copy link

glen-84 commented Feb 1, 2020

@brainkim, I've opened #7512 rather, as I'd like to avoid arguing about pros and cons in a closed issue.

@jwalton
Copy link

jwalton commented Mar 23, 2020

Is there an option to disable "space after function"?

@alexander-akait
Copy link
Member

No options, it is default behavior

@jwalton
Copy link

jwalton commented Mar 23, 2020

Would there be resistance to a PR for adding an option? It's a) ugly and b) adds tons and tons of pointless churn to my existing prettier projects.

@alexander-akait
Copy link
Member

No, it is default behavior

@jwalton
Copy link

jwalton commented Mar 23, 2020

According to Prettier's option philosophy, Prettier doesn't add options when they can be avoided, because ultimately most of those options are just bike shedding, and we shouldn't have debates over bike shedding. This is the core value proposition of Prettier over other code formatters; it's opinionated.

But, what are we to make of this PR, then? Either "space after function keyword" is bike shedding in which case why was this PR merged? Prettier's primary value driver is that it's opinionated, but if that opinion can change on a whim, Prettier is considerably less valuable, and possibly even a liability. This PR is going to add 20K lines of churn to my project and will probably break a number of unmerged PRs, and that's just for me. This PR generates a lot of work for a lot of people for a capricious and arbitrary change. How do I know Prettier isn't going to do something like that again in the future? What if Prettier decides open braces should always be on the next line? Or that indents should always be 8 characters? If Prettier can make this change without providing me with some sort of backwards compatibility to previous versions, how do I know Prettier won't create a bunch of work for me again next release?

Or, alternatively, "space after function keyword" isn't a bike shed change, in which case there should be an option.

@lydell
Copy link
Member

lydell commented Mar 23, 2020

@jwalton Thanks for the feedback, but I’m afraid you’re too late to the game. This has already been discussed to death (and definitely wasn’t “changed on a whim”).

@j-f1 j-f1 changed the title RFC: Always add a space after the function keyword Always add a space after the function keyword Mar 24, 2020
@chrisfosterelli
Copy link

chrisfosterelli commented Mar 24, 2020

Nearly the entirety of this thread's comments seem against it, so I'm not super sure how this concluded with a change. I'm having difficulty finding any reasoning for this change at all, but there is a lot of text here and I'm sure you have some context I don't.

I understand prettier is designed to be opinionated -- that's why I originally loved adopting it. Those opinions matched my own, and gave options when they didn't. In this case, prettier has now made a big change that affects a big chunk of my code (and if #3845 follows a similar pattern it'll be the entirety of my code) so that it no longer lines up with my opinions, and also provided no option for controlling this.

My only resolution here is to leave prettier pinned to an old version permanently, and see how long that works before I find an alternative. I see that the original issue had many more 👍 than 👎, but I suspect you might underestimate the portion of users that actually prefer the original behaviour but just don't follow the repo closely. When making these changes, you're going to hear from primarily people who want that change rather than the people that are happy with the behaviour and don't come to the issue tracker. I mean, I'm here now and reading through all this only because I'm seeing V2 doesn't match my preference. 😁

I totally respect your decision here and that it's your library to make these changes as you see fit, but thought you might appreciate the 2c from someone who is now going to look at alternatives.

@jwalton
Copy link

jwalton commented Mar 24, 2020

but I’m afraid you’re too late to the game

Like most people, I suspect, I'm only finding out about this because I'm trying to upgrade to Prettier 2.x. I had no idea this was coming. So, this is as early to the game as I can get. :)

This has already been discussed to death (and definitely wasn’t “changed on a whim”).

@lydell I'm curious if you could give a quick summation of why this new "space after function" behavior is so much better than the old behavior, then?

@lydell
Copy link
Member

lydell commented Mar 24, 2020

Sorry, I can’t.

@jwalton
Copy link

jwalton commented Mar 24, 2020

@lydell In that case, I'd point out that the funny thing about bike shedding is, you can discuss an issue to death, and instead of finding the one true color to paint all future bike sheds, you can come up with a decision that's largely arbitrary but wasted a lot of time to get to. :) I've read a lot of the history here, and I can't explain why this change is better either, beyond some vague notion of consistency.

At the end of the day, whether there's a space after function or not doesn't matter to me at all. I think it's ugly, but I suspect if I made the switch then in a few months I'd get used to it ad it would just be normal. That's all besides the point, though.

I don't use Prettier because it's the "one true coding style". I use it for the same reason I use any tool; because it saves me time. Today, upgrading to 2.x, Prettier is creating a bunch of work for me and my team - churning about 20K lines of code for seemingly no reason (or at least seemingly flimsy, if well debated reasons). This is literally the opposite of "saving me time".

I've seen a few places where it was suggested this change was fine for 2.0, because it would be a "one time" big churn, and people would deal with it and move on. Except... is it one time? This PR was originally vetoed for exactly the reasons I'm talking about, but Prettier eventually caved in and made this change anyways. If Prettier is willing to do that once, then probably Prettier will do it again, which means Prettier isn't stable - and this is the real problem.

If there was at least an option to disable this, then Prettier would be changing the default behavior but would have an option to make it stable. Without an option, I'm in the same boat as @chrisfosterelli - I have to decide how much future work I'm willing to devote to Prettier's (from an outsider's view) random changes, or I have to pin to 1.19.1 and say "If I'm going to have to accept a bunch of churn, maybe I should just take a bunch of churn and move to a stable replacement."

(And like @chrisfosterelli I'll point out that's totally cool - your project doesn't have to meet my requirements. It's an OSS project so I've got no rights to make any kind of demands. I'm not even willing to fork it because just merging fixes would be more work than I have time for, so my opinion is worth considerably less than $0.02.)

@leizhao4
Copy link

I don't know if Prettier has promised to be so "stable" that no changes can ever be made even during a major release. Programming is a fast-moving field and everything needs to evolve and improve. In my opinion, limiting major changes to major releases is a good balance. I wouldn't be surprised when version 3.0 comes it will bring in some other changes that require us to run it once more. And I believe that's just a small but necessary cost.

@jwalton
Copy link

jwalton commented Mar 25, 2020

@leizhao4 I 100% agree with what you just said... But this is the important part:

And I believe that's just a small but necessary cost.

Was this a necessary cost? I'll ask you the same question; can you give a quick summation of why this new "space after function" behavior is so much better than the old behavior? If I upgrade to 2.x, how will my code be quantifiably better by having a space after every function keyword? Is there some JS trap where not having that space is likely to increase my bug count? Is there some super magic grep command I can now use that I couldn't before?

Because if there's a good reason for it, then yes, this was a necessary cost, and we'll all be the better for adopting the new Prettier version and it's awesome new space-after-function feature. If the reason is "because some fraction of the Prettier user base thought it looked better this way", then even if it's a fairly large fraction, upgrading is a completely pointless and arbitrary cost. Programming is a fast moving field, and evolving and improving are noble goals, but is this an evolution and/or an improvement, or is this a change just for the sake of change?

@leizhao4
Copy link

@jwalton What I'm trying to say is: allowing Prettier to evolve (versus being "stable") is a necessary cost. I hope this is the part that you agree with me. "I just don't want changes" is not a valid argument in my opinion.

Whether this change is for better or worse, on the other hand, can be subjective, and has been extensively discussed in #3847. And it is not the point of my previous post. The TL;DR is: everyone had their opinion but a decision was made.

@jwalton
Copy link

jwalton commented Mar 25, 2020

@leizhao4 I don't want prettier to not change just because I don't like changes. But I do want to know that when prettier makes a change that requires me to spend time to accommodate it, there was a sensible and well reasoned motivation for that change.

@zomp
Copy link

zomp commented Mar 25, 2020

@jwalton There were many reasons. Please read #3847, #1139... It is a bit pointless to write comments here otherwise.

@jwalton
Copy link

jwalton commented Mar 25, 2020

If there were many good reasons to make this change, surely you can sum up the top three in a couple of lines here and save me (and anyone else who is curious about this) all that reading? I'm surprised they weren't summed up in the release notes, to be honest.

@asapach
Copy link

asapach commented Mar 25, 2020

@jwalton, release notes did mention the rationale behind the change: link

@karlhorky
Copy link
Contributor

karlhorky commented Mar 25, 2020

Churn and Major Versions: I think churn caused by a major version is not unacceptable for an opinionated formatting tool. There will always be opinions against and for a specific style, and Prettier needs to try to find some middle ground that is a "pretty" style in the eyes of the majority of the ecosystem.

A Proposal: However, maybe Prettier can make the transition easier for those upgrading to a major version. For example, after upgrading to a Prettier major, let the user know and give them an easy way to upgrade their whole project at once (or change their option, in case there is one).


For example, consider #4102 (the change of quotes to default to single quotes), which may be changed in 3.0.

With some kind of automation or messaging (during install or first run?), the users who upgrade to 3.0 would receive a message that this option has changed and a simple way to upgrade their whole project in one step.

Since this case with quotes also has a corresponding option, there would be a separate step for those users who prefer double quotes, which would add the option to the Prettier config.


I would imagine the CLI could handle such a thing with a new command (possibly also with a version field in prettier.config.js):

prettier migrate

@lipis
Copy link
Member

lipis commented Mar 25, 2020

I'll recommend locking this issue and opening a new one if we really have to. This is done deal.

@phaux
Copy link

phaux commented Mar 25, 2020

To migrate the whole project you just run prettier --write . 🤷

@prettier prettier locked as off-topic and limited conversation to collaborators Mar 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet