Skip to content

Add code formatting presets #893

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

Merged
merged 4 commits into from
Jun 17, 2017
Merged

Add code formatting presets #893

merged 4 commits into from
Jun 17, 2017

Conversation

kapilmb
Copy link

@kapilmb kapilmb commented Jun 17, 2017

Adds the following presets for code formatting:

  • Custom
  • OTBS
  • Allman

Related PRs:
PowerShell/PSScriptAnalyzer#784
PowerShell/PowerShellEditorServices#521

],
"default": "Custom",
"description": "Sets the codeformatting options to follow the given indent style in a way that is compatible with PowerShell syntax. For more information about the brace styles please refer to https://github.com/PoshCode/PowerShellPracticeAndStyle/issues/81."
},
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we point to this or create some documentation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use this link for now until we get our own documentation together.

@@ -412,6 +412,16 @@
"default": false,
"description": "Launches the language service with the /waitForDebugger flag to force it to wait for a .NET debugger to attach before proceeding."
},
"powershell.codeFormatting.preset": {
"type":"string",
"enum": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, I was looking for how to do the enum values and couldn't find it!

@daviwil daviwil added this to the 1.4.0 milestone Jun 17, 2017
@daviwil
Copy link
Contributor

daviwil commented Jun 17, 2017

Nice work!

@daviwil daviwil merged commit 3bcd36d into master Jun 17, 2017
@daviwil daviwil deleted the kapilmb/add-formatting-preset branch June 17, 2017 15:43
@rkeithhill
Copy link
Contributor

Can we add a Stroustrup style which is what most (all?) of our example script uses? http://eslint.org/docs/rules/brace-style

@rkeithhill
Copy link
Contributor

BTW I just did a git pull in both vscode-powershell and PSES but when I try to format code, I get an error indicating I missing a command Invoke-Formatter? Is this something provided by PSES?

@daviwil
Copy link
Contributor

daviwil commented Jun 17, 2017

Ahhh, I didn't look very closely at what Allman was :) Yeah, we should definitely have an additional preset for Stroutsroup. Also, should we rename OTBS to 1TBS? The Wikipedia article prioritizes the latter acronym:

https://en.wikipedia.org/wiki/Indent_style#1TBS

@rkeithhill
Copy link
Contributor

BTW I don't mind adding the support for Stroustrup, if that's OK with you guys. I assuming it will be pretty easy. :-)

@daviwil
Copy link
Contributor

daviwil commented Jun 17, 2017

Invoke-Formatter comes from PSScriptAnalyzer 1.14, but I think Kapil's using some changes that will appear in 1.15. You might need to clone/build PSScriptAnalyzer for it to work

@daviwil
Copy link
Contributor

daviwil commented Jun 17, 2017

Go for it man, that would be great!

@kapilmb
Copy link
Author

kapilmb commented Jun 17, 2017

I went through the indent styles and I see that most PS script are not really Stroustrup but OTBS with Stroustrup else/elseif/catch/finally. The issue here also mentions the same. So maybe calling it Stoustrup can be a bit misleading. I do not worry that much about the naming and style as long it is consistent, but some people can be overzealous about the styles and their names.

@kapilmb
Copy link
Author

kapilmb commented Jun 17, 2017

Regarding OTBS to 1TBS, I am not sure if we name an indentifier that starts with a digit. However we can have a display name that is 1TBS but then that would be an overkill it think.

@kapilmb
Copy link
Author

kapilmb commented Jun 17, 2017

Looks like the Stroustrup style in http://eslint.org/docs/rules/brace-style doesn't treat function declarations in a different way. If we want to use the style defined in the link as reference then I think the Straustrup name for the setting should be ok.

@rkeithhill
Copy link
Contributor

@kapilmb That is what I was thinking. Some folks will probably get a bit pedantic about it but I'm not finding a good "name" for this other than 1TBS sans CuddledElse and that's just too long (and weird). :-)

@kapilmb
Copy link
Author

kapilmb commented Jun 17, 2017

I agree 1TBS sans CuddledElse appears a bit long. I think we just call it Stroustrup and make a note somewhere in the documentation that since all the brace-able nested elements have open braces on same line in the Stroustrup style and since PowerShell allows nested function declaration, for the sake of consistency we put the open brace on the same line as the function name no matter where the function is declared. How does that sound?

@daviwil
Copy link
Contributor

daviwil commented Jun 17, 2017

Sounds good to me!

@rkeithhill
Copy link
Contributor

rkeithhill commented Jun 17, 2017

Sounds good. So I've built PSSA and verified that I'm invoking the one I built but I get an error about the range parameter:

13:68ms> Invoke-Formatter .\DebugTest.ps1 $settings 30,1,36,1
Invoke-Formatter : Invalid Range
Parameter name: range
At line:1 char:1
+ Invoke-Formatter .\DebugTest.ps1 $settings 30,1,36,1
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (Microsoft.Windo...tAnalyzer.Range:Range) [Invoke-Formatter], ArgumentException
    + FullyQualifiedErrorId : FIX_ERROR,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeFormatterCommand

I also get a similar error inside VSCode. I've built the development branch but maybe the up-to-date formatting changes are on another branch?

That said, if it would be easier for you to add this setting, go for it. I originally thought I would be updating something in the VSCode extensions TS files. I'm guessing that the only thing we need to do is copy the CodeFormattingOTBS.psd1 to CodeFormattingStroustrup.psd1 and change this one setting:

    Rules        = @{
        PSPlaceOpenBrace           = @{
            Enable             = $true
            OnSameLine         = $false  # CHANGED from the OTBS value of $true
            NewLineAfter       = $true
            IgnoreOneLineBlock = $true
        }

And update the vscode-powershell package.json/setttings.ts files. Right? Hmm, I could easily do this but I'm a bit nervous about not being able to test the changes. Any idea why the Range parameter error is happening. Do I not have the right "format": startLine, startCol, endLine, endCol?

@kapilmb
Copy link
Author

kapilmb commented Jun 17, 2017

error about the range parameter

Two issues here:

  • The Invoke-Formatter cmdlet doesn't take a file path input yet. You will have to provide it with the content of the file that you want formatted.
  • I just checked debugtest.ps1 and there are only 29 lines in the script. So, the range 30,1,36,1 will not work. Btw, you have the right format regarding startLine,...,endCol.

The formatting changes are there in the development branch. So, something like the following should work.

Invoke-Formatter (Get-Content .\DebugTest.ps1 -raw) $settings 22,1,26,1

The original goal was to just create a settings file in PSScriptAnalyzer and let vscode extension use it as preset, with minor changes to settings.ts and package.json. But I soon realized that the settings files are static and the tabSize and insertSpaces can change from one invocation to another. So, I had to mess some things up on the PSES side as well. We are in a situation now where the formatting setting files in PSSA are irrelevant to the extension. So, if we want to add Stroustrup to the extension, then we would need to change:

  • vscode-powershell

    • package.json
    • settings.ts
  • PSES

    • src/PowerShellEditorServices.Protocol/Server/LanguageServerSettings.cs

On the PSSA-side you are right that we only need to make a copy of the OTBS psd1 file but we need to change the PlaceCloseBrace.NewLineAfter and not PlaceOpenBrace.OnSameLine.

Regarding adding the setting, I am fine with either ways - either you add it or I add it. Let me know what you prefer.

@rkeithhill
Copy link
Contributor

I've submitted a PR for vscode-powershell. I'll submit one shortly for PSES. Thanks for the tip on not using a path. My debugTest.p1 had those extra lines because I added an if/else statement to the bottom of the file - just for testing. :-)

@rkeithhill
Copy link
Contributor

BTW am I getting the settings correctly in PSSA? I'm using:

$settings = iex (gc .\settings\CodeFormattingStroustrup.psd1 | out-string)

which results in $settings containing the hashtable.

@kapilmb
Copy link
Author

kapilmb commented Jun 17, 2017

yeah, I think that should work.

The -Settings parameter takes the following put:

  • string type pointing to a settings (*.psd1) file.
  • string type indicating the preset. This is just the filename without extension for all the files in Engine\Settings\*.psd1
  • a hashtable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants