Add new code formatting preset "OTPS": OTBS with else on new line#2158
Add new code formatting preset "OTPS": OTBS with else on new line#2158o-l-a-v wants to merge 5 commits intoPowerShell:mainfrom
else on new line#2158Conversation
bergmeister
left a comment
There was a problem hiding this comment.
Happy to add it but can you please add a test that specifically tests the OTPS part of else being on new line?
@liamjpeters Are you happy with those properties since you added new ones and potentially other ones in pending PRs? Or would we need to take a note to add certain properties later here?
|
I don't see where I should add a test like that. There seems to be no such test for other presets, only the specific settings seems to be tested. Also, there are some of the tests that make no sense. Like: No comments are involved in those tests. Idea: Should be create a new test file that has all the samble codes from: PoshCode/PowerShellPracticeAndStyle#177 And just say that they are the expected output from the various presets? |
…osing bracket and else
|
I added some suggestions for tests now @bergmeister. 😊 |
There was a problem hiding this comment.
Pull request overview
This PR adds a new code formatting preset called "OTPS" (One True PowerShell Style), which is identical to OTBS but places else on a new line (similar to Stroustrup style). The key difference from OTBS is the NewLineAfter = $true setting on PSPlaceCloseBrace.
Changes:
- New
CodeFormattingOTPS.psd1settings file defining the OTPS preset. - New
CodeFormattingPresets.tests.ps1test file covering all preset cross-conversion scenarios. - Updates to existing brace placement tests to include the OTPS preset.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Engine/Settings/CodeFormattingOTPS.psd1 | Defines the new OTPS formatting preset rules |
| Tests/Rules/CodeFormattingPresets.tests.ps1 | New test file verifying all formatting preset conversions including OTPS |
| Tests/Rules/PlaceOpenBrace.tests.ps1 | Adds OTPS to existing open brace formatting tests |
| Tests/Rules/PlaceCloseBrace.tests.ps1 | Adds new context block testing if/else formatting across all presets |
Comments suppressed due to low confidence (1)
Tests/Rules/CodeFormattingPresets.tests.ps1:1
- The
$OTPSAndStroustrupDefinitiontest fixture inPlaceCloseBrace.tests.ps1uses double-quoted strings ("yes","no"), while the equivalent$OTPSfixture inCodeFormattingPresets.tests.ps1uses single-quoted strings ('yes','no'). If the formatter normalizes string quoting, these fixtures could diverge in meaning or lead to test confusion. Consider aligning the quoting style between the two fixtures for consistency.
# Copyright (c) Microsoft Corporation. All rights reserved.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $OTPSAndStroustrupDefinition = @" | ||
| if (`$true) { | ||
| "yes" | ||
| } | ||
| else { | ||
| "no" | ||
| } | ||
| "@ |
There was a problem hiding this comment.
The variable $OTPSAndStroustrupDefinition implies that this definition is identical for both OTPS and Stroustrup. However, based on the full preset definitions and tests in CodeFormattingPresets.tests.ps1, OTPS and Stroustrup differ in other formatting aspects (e.g., function brace placement, param block spacing). This naming may be misleading in a broader context. Consider naming it more specifically, such as $ElseOnNewLineDefinition, to avoid implying full equivalence between the two presets.
There was a problem hiding this comment.
I don't agree. For this particular test, OTPS and Stroustrup will create the same result.
|
Hmm, may these tests have found a bug in the Stroustrup preset? No, two bugs?
PS > $Allman = @'
enum Color
{
Black,
White
}
function Test-Code
{
[CmdletBinding()]
param
(
[int] $ParameterOne
)
end
{
if (10 -gt $ParameterOne)
{
"Greater"
}
else
{
"Lesser"
}
}
}
'@
PS > Invoke-Formatter -ScriptDefinition $Allman -Settings 'CodeFormattingStroustrup'
enum Color {
Black,
White
}
function Test-Code {
[CmdletBinding()]
param
(
[int] $ParameterOne
)
end {
if (10 -gt $ParameterOne) {
"Greater"
}
else {
"Lesser"
}
}
}Edit: These are probably shortcomings with PSSA? |
All good with me 😊. I should look over the settings presets once #2132 is merged, to add in the enum alignment, but that can wait. Good reminder, thanks! @o-l-a-v - Thanks for the new tests 👍 |
|
If I understand it correctly, PSSA code formatting only handles curly brackets, thus there will be no difference between Stroustrup and this new proposed preset. As the new test file shows, PSSA does not touch If I'm not missing anything here I don't think this proposal makes sense anymore. What do you guys think we should do? Remove the new preset, keep the closing brackets tests, remove or move to disabled the new test file I made for preset tests? |
PR Summary
elseon new line PowerShellEditorServices#2269elseon new line vscode-powershell#5413This PR adds a new code formatting preset, "OTPS" (One True PowerShell Style): OTBS with
elseon new line.Thoughts behind the name:
elseon new line), also in the name (change B with P).Feature request:
elseon a new line #2045Context:
elseon a new line:PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.Tested like so