Skip to content

Code formatting should place cuddled else on new line when openBraceOnSameLine is false #508

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

Closed
Jaykul opened this issue Feb 16, 2017 · 18 comments · Fixed by PowerShell/PSScriptAnalyzer#713
Labels
Issue-Bug A bug to squash.

Comments

@Jaykul
Copy link

Jaykul commented Feb 16, 2017

In an attempt to conform to the Allman style indenting preferred by my team, I set powershell.codeFormatting.newLineAfterOpenBrace": false and Format Document converted this:

   if(Test-Path "C:\Program Files (x86)\PHP\5.6.26\CACert") {
       $PEM = Get-Content $PEM.FullPath -raw
       Add-Content "C:\Program Files (x86)\PHP\5.6.26\CACert\rootCA.pem" $Pem -Encoding ascii
   } else {
       Write-Warning "Failed to deploy PHP cert"
   }

to this:

    if(Test-Path "C:\Program Files (x86)\PHP\5.6.26\CACert")
    {
        $PEM = Get-Content $PEM.FullPath -raw
        Add-Content "C:\Program Files (x86)\PHP\5.6.26\CACert\rootCA.pem" $Pem -Encoding ascii
    } else
    {
        Write-Warning "Failed to deploy PHP cert"
    }

I don't know what you call that half-cuddled else, but I just call it wrong 😉

If people have set "powershell.codeFormatting.openBraceOnSameLine": false, you need to ditch the cuddled else as well -- or add a setting for elseOnSameLine 😀

@daviwil daviwil changed the title What brace style do you call this? Code formatting should place cuddled else on new line when openBraceOnSameLine is false Feb 16, 2017
@daviwil
Copy link
Contributor

daviwil commented Feb 16, 2017

@kapilmb looks like a minor bug in the brace formatting rule

@daviwil daviwil added the Issue-Bug A bug to squash. label Feb 16, 2017
@Jaykul
Copy link
Author

Jaykul commented Feb 16, 2017

P.S.: my vote is to not add the elseOnSameLine setting.

Just do full Allman style for openBraceOnSameLine = false and OTBS (aka K&R) with cuddled else otherwise.

@rkeithhill
Copy link
Contributor

Uh, if openBraceOnSameLine = true I still don't want a cuddled else/elseif/catch/finally. So if the formatting is doing that now, I would prefer a setting disable that.

As long as I can get the formatter to format to:

if ($true) {
    ...
}
else {
    ...
}

I'll be happy. :-)

@kapilmb
Copy link

kapilmb commented Feb 21, 2017

@rkeithhill With PowerShell/PSScriptAnalyzer#713 checkin, the codeformatter will allow only the following options

Style 1

"powershell.codeFormatting.openBraceOnSameLine": true

if (...) {
...
}
else {
...
}

Style 2

"powershell.codeFormatting.openBraceOnSameLine": false

if (...) 
{
...
}
else
{
...
}

Style 3

"powershell.codeFormatting.ignoreOneLineBlock": true

if (...) {...} else {...}

@daviwil
Copy link
Contributor

daviwil commented Feb 21, 2017

And the new NewLineAfter setting is true by default, meaning that cuddled else is opt-in by setting NewLineAfter to false.

@kapilmb
Copy link

kapilmb commented Feb 21, 2017

@daviwil We haven't exposed that switch via codeformatter - Should we expose that here? Then it would look something like this: '"powershell.codeFormatting.newLineAfterCloseBrace": true`

@daviwil
Copy link
Contributor

daviwil commented Feb 21, 2017

Yep, definitely.

@kapilmb
Copy link

kapilmb commented Feb 22, 2017

PR #512 adds the newLineAfterCloseBrace switch .

@Jaykul
Copy link
Author

Jaykul commented Mar 5, 2017

This is a travesty 😜

if (...) {
...
}
else {
...
}

You're choosing "Stroustrup" (style one) over the One True Brace Style? With "cuddled else" like the OTBS, you can safely insert a line (even a blank line) anywhere you like without breaking things. Why push Stroustrup?

@rkeithhill
Copy link
Contributor

@Jaykul What do you think this is - JavaScript? :-) Picking a default brace style is sort of a no-win situation. I "don't believe sermons, fairy tales or stories about a OTBS" (name that movie). :-)

I think the best that can be done is provide settings to allow folks to configure brace style to their own taste. They can configure that setting both per user and per workspace. With that in mind, maybe we need a "powershell.codeFormatting.cuddleBrace": false or something to that effect. My preference would be that it defaults to false.

@ctsstc
Copy link

ctsstc commented May 23, 2017

How can I get stroustrup style for my if else statement blocks, I'm going crazy here.

@daviwil
Copy link
Contributor

daviwil commented May 23, 2017

@ctsstc follow this issue, we're fixing it soon: #740

@daviwil
Copy link
Contributor

daviwil commented May 23, 2017

But before I go too far in assuming what you mean, can you give an example of what you're looking for?

@ctsstc
Copy link

ctsstc commented May 25, 2017

@daviwil http://eslint.org/docs/rules/brace-style

Stroustrup

if (foo) {
  bar();
}
else {
  baz();
}

@Jaykul
Copy link
Author

Jaykul commented May 25, 2017

@ctsstc Stroustrup and Allman are the only styles it does right now. Worst of the worst. 😛

I think it works like this:

Stroustrup

    "powershell.codeFormatting.openBraceOnSameLine": true,
    "powershell.codeFormatting.newLineAfterOpenBrace": true,
    "powershell.codeFormatting.newLineAfterCloseBrace": true,

Allman

    "powershell.codeFormatting.openBraceOnSameLine": false,
    "powershell.codeFormatting.newLineAfterOpenBrace": true,
    "powershell.codeFormatting.newLineAfterCloseBrace": true,

@daviwil
Copy link
Contributor

daviwil commented May 25, 2017

@ctsstc your desired style is the default in our code formatter. Can you give an example of where our default formatting does not follow that style?

@ctsstc
Copy link

ctsstc commented May 31, 2017

Oh wow, I've failed, I thought this was the main VSCode Repo; /facepalm...
And I was indeed trying to get it to do that for JS. It's hard to tell what's doing what sometimes when you've got so many extensions installed. I have no clue what a vanilla experience is or if I have conflicting extensions at some point.

@daviwil
Copy link
Contributor

daviwil commented May 31, 2017

No problem! Yeah, that's one of the downsides I suppose, each extension has its own way of configuring code formatting (if it's even supported). You might take a look at the "javascript.format.*" settings and see if you can find the setting you need there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug A bug to squash.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants