Skip to content

Commit a7d23f7

Browse files
authored
Fix PlaceOpenBrace rule correction to take comment at the end of line into account (#929)
* Fix OpenBrace rule to take comment at the end of line into account * add test case when converting from a brace style where the brace is on the next line to a brace style where the brace is on the same line as suggested in the PR. Added more assertions for an integration test with code formatting styles * fix CI failures by putting the new invoke-formatter in their own context block (they screwed up following tests for some reason).
1 parent 021711e commit a7d23f7

File tree

2 files changed

+67
-3
lines changed

2 files changed

+67
-3
lines changed

Rules/PlaceOpenBrace.cs

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
1515
{
1616
/// <summary>
17-
/// A class to walk an AST to check for violation.
17+
/// A formatting rule about whether braces should start on the same line or not.
1818
/// </summary>
1919
#if !CORECLR
2020
[Export(typeof(IScriptRule))]
@@ -201,14 +201,23 @@ private IEnumerable<DiagnosticRecord> FindViolationsForBraceShouldBeOnSameLine(
201201
&& tokens[k - 1].Kind == TokenKind.NewLine
202202
&& !tokensToIgnore.Contains(tokens[k]))
203203
{
204+
var precedingExpression = tokens[k - 2];
205+
Token optionalComment = null;
206+
// If a comment is before the open brace, then take the token before the comment
207+
if (precedingExpression.Kind == TokenKind.Comment && k > 2)
208+
{
209+
precedingExpression = tokens[k - 3];
210+
optionalComment = tokens[k - 2];
211+
}
212+
204213
yield return new DiagnosticRecord(
205214
GetError(Strings.PlaceOpenBraceErrorShouldBeOnSameLine),
206215
tokens[k].Extent,
207216
GetName(),
208217
GetDiagnosticSeverity(),
209218
fileName,
210219
null,
211-
GetCorrectionsForBraceShouldBeOnSameLine(tokens[k - 2], tokens[k], fileName));
220+
GetCorrectionsForBraceShouldBeOnSameLine(precedingExpression, optionalComment, tokens[k], fileName));
212221
}
213222
}
214223
}
@@ -336,17 +345,19 @@ private int GetStartColumnNumberOfTokenLine(Token[] tokens, int refTokenPos)
336345

337346
private List<CorrectionExtent> GetCorrectionsForBraceShouldBeOnSameLine(
338347
Token precedingExpression,
348+
Token optionalCommentOfPrecedingExpression,
339349
Token lCurly,
340350
string fileName)
341351
{
342352
var corrections = new List<CorrectionExtent>();
353+
var optionalComment = optionalCommentOfPrecedingExpression != null ? $" {optionalCommentOfPrecedingExpression}" : string.Empty;
343354
corrections.Add(
344355
new CorrectionExtent(
345356
precedingExpression.Extent.StartLineNumber,
346357
lCurly.Extent.EndLineNumber,
347358
precedingExpression.Extent.StartColumnNumber,
348359
lCurly.Extent.EndColumnNumber,
349-
precedingExpression.Text + " " + lCurly.Text,
360+
$"{precedingExpression.Text} {lCurly.Text}{optionalComment}",
350361
fileName));
351362
return corrections;
352363
}

Tests/Rules/PlaceOpenBrace.tests.ps1

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,59 @@ function foo ($param1)
3535
}
3636
}
3737

38+
Context "Handling of comments when using Invoke-Formatter" {
39+
It "Should correct violation when brace should be on the same line" {
40+
$scriptDefinition = @'
41+
foreach ($x in $y)
42+
{
43+
Get-Something
44+
}
45+
'@
46+
$expected = @'
47+
foreach ($x in $y) {
48+
Get-Something
49+
}
50+
'@
51+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected
52+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingStroustrup' | Should -Be $expected
53+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingOTBS' | Should -Be $expected
54+
}
55+
56+
It "Should correct violation when brace should be on the same line and take comment into account" {
57+
$scriptDefinition = @'
58+
foreach ($x in $y) # useful comment
59+
{
60+
Get-Something
61+
}
62+
'@
63+
$expected = @'
64+
foreach ($x in $y) { # useful comment
65+
Get-Something
66+
}
67+
'@
68+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected
69+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingStroustrup' | Should -Be $expected
70+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingOTBS' | Should -Be $expected
71+
}
72+
73+
It "Should correct violation when the brace should be on the next line and take comment into account" {
74+
$scriptDefinition = @'
75+
foreach ($x in $y) # useful comment
76+
{
77+
Get-Something
78+
}
79+
'@
80+
$expected = @'
81+
foreach ($x in $y) { # useful comment
82+
Get-Something
83+
}
84+
'@
85+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected
86+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingStroustrup' | Should -Be $expected
87+
Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingOTBS' | Should -Be $expected
88+
}
89+
}
90+
3891
Context "When an open brace must be on the same line in a switch statement" {
3992
BeforeAll {
4093
$def = @'

0 commit comments

Comments
 (0)