From 6f9349442ea4a0481e3d4902f9375ca5a6a8d261 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 11 Mar 2018 19:00:38 +0000 Subject: [PATCH 1/3] Fix OpenBrace rule to take comment at the end of line into account --- Rules/PlaceOpenBrace.cs | 29 ++++++++++++++------------ Tests/Rules/PlaceOpenBrace.tests.ps1 | 31 ++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/Rules/PlaceOpenBrace.cs b/Rules/PlaceOpenBrace.cs index eac527f46..5a74626d9 100644 --- a/Rules/PlaceOpenBrace.cs +++ b/Rules/PlaceOpenBrace.cs @@ -1,12 +1,5 @@ -// Copyright (c) Microsoft Corporation. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. using System; using System.Collections.Generic; @@ -14,7 +7,6 @@ using System.ComponentModel.Composition; #endif using System.Globalization; -using System.Linq; using System.Management.Automation.Language; using System.Text; using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; @@ -22,7 +14,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { /// - /// A class to walk an AST to check for violation. + /// A formatting rule about whether braces should start on the same line or not. /// #if !CORECLR [Export(typeof(IScriptRule))] @@ -209,6 +201,15 @@ private IEnumerable FindViolationsForBraceShouldBeOnSameLine( && tokens[k - 1].Kind == TokenKind.NewLine && !tokensToIgnore.Contains(tokens[k])) { + var precedingExpression = tokens[k - 2]; + Token optionalComment = null; + // If a comment is before the open brace, then take the token before the comment + if (precedingExpression.Kind == TokenKind.Comment && k > 2) + { + precedingExpression = tokens[k - 3]; + optionalComment = tokens[k - 2]; + } + yield return new DiagnosticRecord( GetError(Strings.PlaceOpenBraceErrorShouldBeOnSameLine), tokens[k].Extent, @@ -216,7 +217,7 @@ private IEnumerable FindViolationsForBraceShouldBeOnSameLine( GetDiagnosticSeverity(), fileName, null, - GetCorrectionsForBraceShouldBeOnSameLine(tokens[k - 2], tokens[k], fileName)); + GetCorrectionsForBraceShouldBeOnSameLine(precedingExpression, optionalComment, tokens[k], fileName)); } } } @@ -344,17 +345,19 @@ private int GetStartColumnNumberOfTokenLine(Token[] tokens, int refTokenPos) private List GetCorrectionsForBraceShouldBeOnSameLine( Token precedingExpression, + Token optionalCommentOfPrecedingExpression, Token lCurly, string fileName) { var corrections = new List(); + var optionalComment = optionalCommentOfPrecedingExpression != null ? $" {optionalCommentOfPrecedingExpression}" : string.Empty; corrections.Add( new CorrectionExtent( precedingExpression.Extent.StartLineNumber, lCurly.Extent.EndLineNumber, precedingExpression.Extent.StartColumnNumber, lCurly.Extent.EndColumnNumber, - precedingExpression.Text + " " + lCurly.Text, + $"{precedingExpression.Text} {lCurly.Text}{optionalComment}", fileName)); return corrections; } diff --git a/Tests/Rules/PlaceOpenBrace.tests.ps1 b/Tests/Rules/PlaceOpenBrace.tests.ps1 index e72278b81..bc7ded34a 100644 --- a/Tests/Rules/PlaceOpenBrace.tests.ps1 +++ b/Tests/Rules/PlaceOpenBrace.tests.ps1 @@ -33,6 +33,37 @@ function foo ($param1) It "Should mark only the open brace" { $violations[0].Extent.Text | Should -Be '{' } + + It "Should correct violation" { + $scriptDefinition = @' +foreach ($x in $y) +{ + Get-Something +} +'@ + $expected = @' +foreach ($x in $y) { + Get-Something +} +'@ + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected + } + + It "Should correct violation and take comment into account" { + $scriptDefinition = @' +foreach ($x in $y) # useful comment +{ + Get-Something +} +'@ + $expected = @' +foreach ($x in $y) { # useful comment + Get-Something +} +'@ + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected + } + } Context "When an open brace must be on the same line in a switch statement" { From ea0cb6d11c409da097198dd6f0713aa0882b6553 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 23 Mar 2018 18:33:31 +0000 Subject: [PATCH 2/3] 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 --- Tests/Rules/PlaceOpenBrace.tests.ps1 | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/Tests/Rules/PlaceOpenBrace.tests.ps1 b/Tests/Rules/PlaceOpenBrace.tests.ps1 index bc7ded34a..f9ba0f3f8 100644 --- a/Tests/Rules/PlaceOpenBrace.tests.ps1 +++ b/Tests/Rules/PlaceOpenBrace.tests.ps1 @@ -62,6 +62,8 @@ foreach ($x in $y) { # useful comment } '@ Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingStroustrup' | Should -Be $expected + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingOTBS' | Should -Be $expected } } @@ -141,5 +143,22 @@ function foo { } It "Should mark only the open brace" { $violations[0].Extent.Text | Should -Be '{' } + + It "Should correct violation and take comment into account" { + $scriptDefinition = @' +foreach ($x in $y) # useful comment +{ + Get-Something +} +'@ + $expected = @' +foreach ($x in $y) { # useful comment + Get-Something +} +'@ + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingStroustrup' | Should -Be $expected + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingOTBS' | Should -Be $expected + } } } From fc5f777a5fecfc84ceb31e60478c9c0ef578f042 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Fri, 23 Mar 2018 19:06:02 +0000 Subject: [PATCH 3/3] fix CI failures by putting the new invoke-formatter in their own context block (they screwed up following tests for some reason). --- Tests/Rules/PlaceOpenBrace.tests.ps1 | 41 +++++++++++++++------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/Tests/Rules/PlaceOpenBrace.tests.ps1 b/Tests/Rules/PlaceOpenBrace.tests.ps1 index f9ba0f3f8..b1ec42c8d 100644 --- a/Tests/Rules/PlaceOpenBrace.tests.ps1 +++ b/Tests/Rules/PlaceOpenBrace.tests.ps1 @@ -33,8 +33,10 @@ function foo ($param1) It "Should mark only the open brace" { $violations[0].Extent.Text | Should -Be '{' } + } - It "Should correct violation" { + Context "Handling of comments when using Invoke-Formatter" { + It "Should correct violation when brace should be on the same line" { $scriptDefinition = @' foreach ($x in $y) { @@ -47,9 +49,11 @@ foreach ($x in $y) { } '@ Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingStroustrup' | Should -Be $expected + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingOTBS' | Should -Be $expected } - It "Should correct violation and take comment into account" { + It "Should correct violation when brace should be on the same line and take comment into account" { $scriptDefinition = @' foreach ($x in $y) # useful comment { @@ -66,6 +70,22 @@ foreach ($x in $y) { # useful comment Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingOTBS' | Should -Be $expected } + It "Should correct violation when the brace should be on the next line and take comment into account" { + $scriptDefinition = @' +foreach ($x in $y) # useful comment +{ + Get-Something +} +'@ + $expected = @' +foreach ($x in $y) { # useful comment + Get-Something +} +'@ + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingStroustrup' | Should -Be $expected + Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingOTBS' | Should -Be $expected + } } Context "When an open brace must be on the same line in a switch statement" { @@ -143,22 +163,5 @@ function foo { } It "Should mark only the open brace" { $violations[0].Extent.Text | Should -Be '{' } - - It "Should correct violation and take comment into account" { - $scriptDefinition = @' -foreach ($x in $y) # useful comment -{ - Get-Something -} -'@ - $expected = @' -foreach ($x in $y) { # useful comment - Get-Something -} -'@ - Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected - Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingStroustrup' | Should -Be $expected - Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings 'CodeFormattingOTBS' | Should -Be $expected - } } }