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

Feature Request: breakBeforeElse #840

Open
vishnevskiy opened this issue Mar 1, 2017 · 35 comments

Comments

Projects
None yet
@vishnevskiy
Copy link

commented Mar 1, 2017

I know the goal of prettier is to standardize on a style, but it is also difficult since this did not come out at the same time as the language.

Our team uses the exact same style as prettier except placement for else statements. It is effectively blocking us from adopting this awesome library.

We prefer break before else to allow for cleaner comments.

// This does something
if (a) {
}
// This does something else if
else if {

}
// This does something else
else {

}

Which is nicer than.

// This does something
if (a) {

// This does something else if
} else if {

// This does something else
} else {

}

However this is preference and would be awesome if this was added as an option to prettier.

@vjeux showed how easy it is to do #837 and apparently @jlongster likes it :) https://twitter.com/jlongster/status/834407714965057540

@vjeux

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2017

Related discussion #807

@SimenB

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2017

Is this the discussion issue mentioned in #837?

If so, here goes: I would prefer it to remain like it is, because it's AFAIK the most prevalent style (it's at least 1TBS), and the default in airbnb, google and jquery code styles (to name a few).

For a real argument besides popularity, I like that related block are connected together. Having else (if) being on the same line as the closing brace of the preceding if, makes it very clear at a glance that the else (if) block is connected to it, instead of being it's own separate entity.
Possible bonus: IDEs providing collapsing of blocks can then naturally collapse the whole if-chain, which doesn't really make sense if there's a newline between.

However, I wouldn't be unhappy if an exception was made for a comment, so this was valid:

if (allIsWell) {
  // Live happily ever after
}
// We only get here Ragnarok is upon us
else {
  // Put on your swimsuits
}

SimenB added a commit to SimenB/stylint that referenced this issue Mar 4, 2017

Use prettier for code style
Not happy about the comments above `else`s

Might want to revert this commit and redo the conversion.
See prettier/prettier#840
@Kerrick

This comment has been minimized.

Copy link

commented Apr 14, 2017

This feels like one of those "I can't adopt prettier unless" options that was mentioned in the 1.0 release blog post. I'm in the die-hard camp of else being on its own line for the sake of our eyes scanning down a single column to see if there is an else block. I can't really see myself adopting prettier without a breakBeforeElse option.

@jlongster

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

I do think that this style is better, but our current style is by far the move prevalent style. As there is a clear majority, and this is an issue that only affects a smaller percentage of code (not like semicolons, etc) I don't think we should have a configuration option for it. If we opened up something at this level, there are many other things we should add config options for as well. But we need to make some choices and this one, in my opinions, falls in that camp.

@rhengles

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

@Kerrick As mentioned on #837, breakBeforeElse is supported on arijs/prettier-miscellaneous, and its goal is to support minor features/extra options not picked by Prettier.

@vjeux

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2017

We are past 1.0 and are not going to do any major formatting changes to prettier or add more options. Feel free to use prettier-miscellaneous if you do want it.

@j-f1

This comment has been minimized.

Copy link
Member

commented Aug 19, 2018

See also #4986.

@JoshMcCullough

This comment has been minimized.

Copy link

commented Jan 28, 2019

I guess I don't understand why the maintainers here are so against giving us the option to put else and else if on a new line. But it sounds like nothing will be done?

Meanwhile my dumb brain will continue to make me move else/else-if to its own line only for that work to be undone when I commit my changes.

@duailibe

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

@JoshMcCullough We won't add options unless extremely necessary. Please read https://prettier.io/docs/en/option-philosophy.html

@j-f1

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

Also, we now support the formatting in the issue, if you put a comment between } and else:

Prettier 1.16.1
Playground link

--parser babylon

Input:

// This does something
if (a) {
}
// This does something else if
else if (b) {

}
// This does something else
else {

}

Output:

// This does something
if (a) {
}
// This does something else if
else if (b) {
}
// This does something else
else {
}
@JoshMcCullough

This comment has been minimized.

Copy link

commented Jan 29, 2019

Okay, fair enough. I misunderstood what prettier was intended to be used for. I'm was thinking something more like editor config. Thanks.

PS: IMO the prettier team went the wrong way on the if/else-if/else choice. ;-)

@nunovieira

This comment has been minimized.

Copy link

commented Mar 8, 2019

Is this still under discussion despite being closed? I hope so.
This should be an option. Adding a comment to break } and else is a crappy workaround.

@phaux phaux referenced a pull request that will close this issue Mar 30, 2019

Open

WIP: Add option to choose between 1TBS and Stroustrup brace style #6017

1 of 4 tasks complete
@phaux

This comment has been minimized.

Copy link

commented Mar 30, 2019

I just changed few lines to add this feature in #6017. Can we discuss it again?

@lydell

This comment has been minimized.

Copy link
Collaborator

commented Mar 30, 2019

We could just as well keep the issue open, like we do with several other discussions/option requests.

@lydell lydell reopened this Mar 30, 2019

@duailibe duailibe removed the keep-unlocked label Apr 1, 2019

@jhildenbiddle

This comment has been minimized.

Copy link

commented Apr 11, 2019

Adding another vote for breakBeforeElse.

I've read the Option Philosophy and appreciate the goals. I just happen to be in the "will not adopt without this option" camp and believe this option should be considered.

I noticed the following on the Option Philosophy page:

--arrow-parens was added after – at the time – huge demand. Prettier has to strike a balance between ideal goals and listening to the community.

The linked issue has 62 up votes and 12 down votes. The --jsx-single-quote option was also added "after great demand". This issue currently has 40 up votes and 0 down votes. Is there a threshold that needs to be crossed before a new option is considered?

@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Why you need this option? Please show to us real code with real problems

@jhildenbiddle

This comment has been minimized.

Copy link

commented Apr 11, 2019

@evilebottnawi --

Sure. Just imagine the same code and problems used to justify including options like --use-tabs and --tab-width.

By far the biggest reason for adopting Prettier is to stop all the on-going debates over styles.

Makes sense. You've got to pick your battles. So why not be "opinionated" about two of the most—if not the two most—frequently debated style choices among development teams? Prettier could declare "spaces not tabs, two spaces not four" in an attempt to "stop all the on-going debates over styles", but doing so would mean a lot of people would never adopt prettier so an exception was made. Similarly, I listed two options above that were added due to "huge demand" and "great demand", so there is a history of adding options to prettier when interest is high enough.

My intention isn't to be argumentative. As I said, I appreciate the goals of Prettier and understand that new options must be very carefully considered. The question I posed seems fair given the history of demand driving the introduction of new options: Is there a threshold that needs to be crossed before a new option is considered? Since that's a pretty open-ended question, let me pose a different one: if--arrow-parens was added due to "huge demand" with 62 up votes, will this option be considered if this issue meets or exceeds the same number of up votes?

Prettier has to strike a balance between ideal goals and listening to the community.

Yep. 😄

@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Is there a threshold that needs to be crossed before a new option is considered?

Yes, real code examples and real problems. Voting is just clicking on emoji. Many developers don't know what they want and don't know why do they need. Many of them say "In our company we love this style", but prettier more them somebody company. Therefore, we do not add options because someone else wanted something.

I'm pretty sure that if i will open issue about adding space after ! in if (! myVar) {} I will get same votes as here. So votes it is meta. It is also worth saying that many developers like to argue, but when the time comes to implementation they disappear. Words is simple, code is difficult.

The fastest and easiest way to fix, change or add something in prettier is show code with real problems, even a vote is not needed.

P.S.

I already get 👎 on prev post (10 min) 😄 Seriously? What is wrong to provide examples?

@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

if --arrow-parens was added due to "huge demand" with 62 up votes, will this option be considered if this issue meets or exceeds the same number of up votes?

Because using:

something => something * something

and

(something, other) => something * other

is inconsistently and it is real problem. And i think we remove this option in 2 version in favor to alway use ().

@jhildenbiddle

This comment has been minimized.

Copy link

commented Apr 12, 2019

Guess what else is inconsistent?

if (a) {

}
// Comment
else if (b) {

} else {

}

And what “real problem” does the lack of parens in arrow functions cause? I’m going to have insist on real code demonstrating a real problem here, @evilebottnawi. ;)

@j-f1

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@jhildenbiddle How often does that kind of code come up though?

@jhildenbiddle

This comment has been minimized.

Copy link

commented Apr 12, 2019

For me, fairly often because I tend to summarize behavior above the if or else statement when the content of the block is lengthy or complex. I know I could just add a comment before every else block to see the results I’m looking for, but then I’m manually formatting code so prettier can automatically format my code.

The same inconsistency is tough to ignore when an if/else block with comments (as described above) is near an if/else block without comments:

// Comment...
if (a) {

} 
// Comment...
else {

}

if (b) {

} else {

}

Hope this provides better context for the request.

@lydell

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2019

We're in the process of updating the option philosophy page with everything we've learned since the last revision, so don't spend too much time quoting from it and making arguments based on it.

@nunovieira

This comment has been minimized.

Copy link

commented Apr 12, 2019

Here are some examples why this option could make for better code:

if (something) {
  some statements
  some long statement
}
else if (something_else) {
  some other statements
  another long statement
}
else {
  the end
}

try {
  dangerous code
}
catch (error) {
  phew, saved it
}

do {
  some statements
  some long statement
}
while (something)

Without this option, the generated code is compact but less readable.

@JoshMcCullough

This comment has been minimized.

Copy link

commented Apr 12, 2019

@evilebottnawi looks like we probably can't show you a good example because your mind is already made up. Your claim that parens around single lambda parameters "is real problem" IMO is not. A lambda is perfectly -- and probably more -- readable without the parens around a single param.

Whereas else-on-same-line does cause issues e.g. when you are commenting each if block to indicate/clarify the clause, before the if statement (as already demonstrated multiple times in this thread). Else on the same line makes me cringe every time I write some beautiful code just to have prettier un-pretty it (in this case). I agree with most/all of the other things prettier does, this one I can't agree with.

I understand the aversion to add new/more options, but seriously ... Just Do It™ (Nike).

@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

your mind is already made up

Who says this? I just listen community. Any innovation in prettier is always a dispute. What is the problem to solve this with eslint?

I just want to hear all the possible problems to see if we need it or not.

@jhildenbiddle

This comment has been minimized.

Copy link

commented Apr 13, 2019

Regarding the consistency issue:

// Comment
if (a) {
}
// Comment
else if (b) {
} 
else {
}

Output:

// Comment
if (a) {
}
// Comment
else if (b) {
} else {
}

Demo

The current behavior introduces a new debate for teams to have over how to write comments for these blocks since devs can individually control Prettier's output based on how they write their comments. This not only exacerbates the consistency and debate issues Prettier aims to solve, but it can have the unintended side effect of influencing how devs provide assistance through comments in their code (too much or too little, to force a specific style).

Consider the debate:

  • Should we just be okay with Prettier's mix of 1TBS and stroustrup styles?
  • Should we never comment above else statements for consistent 1TBS style?
  • Should we always comment above else statements to force consistent stroustrup style?
  • Should we allow mixing of styles, but never in the same if/else block?
// YES (1TBS)
if (a) {
} else {
}

// YES (stroustrup)
if (a) {
}
// Important comment
else if (b) {
}
// Unimportant comment added for styling
else {
}

// NO (mixed)
if (a) {
}
// Comment
else if (b) {
} else { 
}

Providing a breakBeforeElse option addresses these issues by ensuring if/else statements and their surrounding comments are always formatted the same way regardless of how comments are authored. It's actually the only way to enforce a single, consistent style for if/else blocks (assuming you don't consider the current behavior "consistently" mixed) which is one reason people (like myself) prefer it.

As others have mentioned, the option also produces better diffs and addresses code-folding issues with some IDEs. This comment is lengthy enough already so I won't dive into those.

Finally, it's been mentioned before (here, here), but these same arguments apply to try/catch, do/while, etc. blocks as well. Just thought I'd put that out there to make some people's head explode. 😩 💣

@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

For me it is bug:

// Comment
if (a) {
}
// Comment
else if (b) {
} else {
}

If comments present we should print:

// Comment
if (a) {
}
// Comment
else if (b) {
} 
else {
}
@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

@suchipi

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

I don't think we need to change this

@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

@suchipi why? with comments it is looks very ugly, when i write parsers/lexers/etc i always if comments between if/esle if/else and difference styles branches look very ugly

@suchipi

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

I don't like that adding or removing a comment could change the diff elsewhere in the file. It could create merge conflicts that would be frustrating for people.

@JoshMcCullough

This comment has been minimized.

Copy link

commented Apr 14, 2019

@suchipi Comments shouldn't change the position of the else statement. That's why we're asking for a new option to always put else on a new line. That way it's there regardless of any preceding comment.

@jhildenbiddle

This comment has been minimized.

Copy link

commented Apr 14, 2019

I agree with @suchipi. @JoshMcCullough is also correct.

To clarify, I believe @suchipi is referring to @evilebottnawi's suggestion of applying breakBeforeElse automatically to individual if/else if/else statements when comments require doing so to enforce a consistent style within that statement (correct me if I have this wrong, @evilebottnawi). The issue with this approach for me—in addition to the diff issue @suchipi mentioned—is that styles will still be inconsistent throughout the codebase:

if (a) {
}
// Comment
else if (b) {
}
else {
}

if (x) {
} else if (y) {
} else {
}

The option being requested by myself and others (this thread, #837, #2573, #3084, #3537, #3832, #4610, #4986, #6017, and likely others I'm missing) is to always apply breakBeforeElse (as @JoshMcCullough pointed out).

I'd also propose that anyone concerned about better diffs and merging should be in favor of breakBeforeElse, not against it.

For those that suggest anyone who cares about this issue simply adopt prettier-miscellaneous or eslint + brace-style, consider that the former hasn't been updated in over two years and the latter means formatting code twice and dealing with potential conflicts as a result (e.g. format-on-save in your editor).

@evilebottnawi

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Yes, always break if you have comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.