Merge pull request #713 from PowerShell/kapilmb/newline-close-brace

Add new line after close brace

Author: Kapil Borle

Engine/Generic/CorrectionExtent.cs

@@ -128,6 +128,21 @@ public CorrectionExtent(
 
         }
 
+        public CorrectionExtent(
+            IScriptExtent violationExtent,
+            string replacementText,
+            string filePath)
+            : this(
+                violationExtent.StartLineNumber,
+                violationExtent.EndLineNumber,
+                violationExtent.StartColumnNumber,
+                violationExtent.EndColumnNumber,
+                replacementText,
+                filePath)
+        {
+
+        }
+
         private void ThrowIfInvalidArguments()
         {
             ThrowIfNull<string>(text, "text");

RuleDocumentation/PlaceCloseBrace.md

@@ -13,6 +13,7 @@ Close brace placement should follow a consistent style. It should be on a new li
             Enable = $true
             NoEmptyLineBefore = $false
             IgnoreOneLineBlock = $true
+            NewLineAfter = $true
         }
     }
 ```
@@ -28,4 +29,7 @@ Create violation if there is an empty line before a close brace.
 #### IgnoreOneLineBlock: bool (Default value is `$true`)
 Indicates if close braces in a one line block should be ignored or not.
 E.g. $x = if ($true) { "blah" } else { "blah blah" }
-In the above example, if the property is set to true then the rule will not fire a violation.
+In the above example, if the property is set to true then the rule will not fire a violation.
+
+#### NewLineAfter: bool (Default value is `$true`)
+Indicates if a new line should follow a close brace. If set to true a close brace should be followed by a new line.

Rules/PlaceCloseBrace.cs

@@ -31,7 +31,7 @@ public class PlaceCloseBrace : ConfigurableRule
         /// <summary>
         /// Indicates if there should or should not be an empty line before a close brace.
         ///
-        /// Default value if false.
+        /// Default value is false.
         /// </summary>
         [ConfigurableRuleProperty(defaultValue:false)]
         public bool NoEmptyLineBefore { get; protected set; }
@@ -42,11 +42,21 @@ public class PlaceCloseBrace : ConfigurableRule
         /// In the above example, if the property is set to true then the rule will
         /// not fire a violation.
         ///
-        /// Default value if true.
+        /// Default value is true.
         /// </summary>
         [ConfigurableRuleProperty(defaultValue: true)]
         public bool IgnoreOneLineBlock { get; protected set; }
 
+        /// <summary>
+        /// Indicates if a new line should follow a close brace.
+        ///
+        /// If set to true a close brace should be followed by a new line.
+        ///
+        /// Default value is true.
+        /// </summary>
+        [ConfigurableRuleProperty(defaultValue: true)]
+        public bool NewLineAfter { get; protected set; }
+
         private HashSet<Token> tokensToIgnore;
 
         /// <summary>
@@ -121,6 +131,13 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
                                 GetViolationForBraceShouldNotFollowEmptyLine(tokens, k, openBracePos, fileName),
                                 ref diagnosticRecords);
                         }
+
+                        if (NewLineAfter)
+                        {
+                            AddToDiagnosticRecords(
+                                GetViolationForBraceShouldHaveNewLineAfter(tokens, k, openBracePos, fileName),
+                                ref diagnosticRecords);
+                        }
                     }
                     else
                     {
@@ -244,6 +261,22 @@ private List<CorrectionExtent> GetCorrectionsForBraceShouldNotFollowEmptyLine(
             return corrections;
         }
 
+        private List<CorrectionExtent> GetCorrectionsForBraceShouldHaveNewLineAfter(
+            Token[] tokens,
+            int closeBracePos,
+            int openBracePos,
+            string fileName)
+        {
+            var corrections = new List<CorrectionExtent>();
+            var nextToken = tokens[closeBracePos + 1];
+            var closeBraceToken = tokens[closeBracePos];
+            corrections.Add(new CorrectionExtent(
+                closeBraceToken.Extent,
+                closeBraceToken.Text + Environment.NewLine,
+                fileName));
+            return corrections;
+        }
+
         private string GetIndentation(Token[] tokens, int closeBracePos, int openBracePos)
         {
             // if open brace on a new line by itself, use its indentation
@@ -271,7 +304,40 @@ private string GetIndentation(Token[] tokens, int closeBracePos, int openBracePo
             return new String(' ', firstTokenOnOpenBraceLine.Extent.StartColumnNumber - 1);
         }
 
-        private DiagnosticRecord GetViolationForBraceShouldBeOnNewLine(Token[] tokens, int closeBracePos, int openBracePos, string fileName)
+        private DiagnosticRecord GetViolationForBraceShouldHaveNewLineAfter(
+            Token[] tokens,
+            int closeBracePos,
+            int openBracePos,
+            string fileName)
+        {
+            var expectedNewLinePos = closeBracePos + 1;
+            if (tokens.Length > 1 && tokens.Length > expectedNewLinePos)
+            {
+                var closeBraceToken = tokens[closeBracePos];
+                if (tokens[expectedNewLinePos].Kind != TokenKind.NewLine
+                    && tokens[expectedNewLinePos].Kind != TokenKind.Comment
+                    && !tokensToIgnore.Contains(tokens[closeBracePos]))
+                    {
+
+                    return new DiagnosticRecord(
+                        GetError(Strings.PlaceCloseBraceErrorShouldFollowNewLine),
+                        closeBraceToken.Extent,
+                        GetName(),
+                        GetDiagnosticSeverity(),
+                        fileName,
+                        null,
+                        GetCorrectionsForBraceShouldHaveNewLineAfter(tokens, closeBracePos, openBracePos, fileName));
+                    }
+            }
+
+            return null;
+        }
+
+        private DiagnosticRecord GetViolationForBraceShouldBeOnNewLine(
+            Token[] tokens,
+            int closeBracePos,
+            int openBracePos,
+            string fileName)
         {
             if (tokens.Length > 1 && tokens.Length > closeBracePos)
             {

Rules/Strings.resx

@@ -903,6 +903,9 @@
   <data name="PlaceCloseBraceErrorShouldNotFollowEmptyLine" xml:space="preserve">
     <value>Close brace does not follow a non-empty line.</value>
   </data>
+  <data name="PlaceCloseBraceErrorShouldFollowNewLine" xml:space="preserve">
+    <value>Close brace does not follow a new line.</value>
+  </data>
   <data name="UseConsistentIndentationName" xml:space="preserve">
     <value>UseConsistentIndentation</value>
   </data>

Tests/Rules/PlaceCloseBrace.tests.ps1

@@ -1,8 +1,14 @@
-Import-Module PSScriptAnalyzer
+$directory = Split-Path -Parent $MyInvocation.MyCommand.Path
+$testRootDirectory = Split-Path -Parent $directory
+
+Import-Module PSScriptAnalyzer
+Import-Module (Join-Path $testRootDirectory "PSScriptAnalyzerTestHelper.psm1")
+
 $ruleConfiguration = @{
     Enable = $true
     NoEmptyLineBefore = $true
     IgnoreOneLineBlock = $true
+    NewLineAfter = $true
 }
 
 $settings = @{
@@ -19,6 +25,9 @@ Describe "PlaceCloseBrace" {
 function foo {
     Write-Output "close brace not on a new line"}
 '@
+            $ruleConfiguration.'NoEmptyLineBefore' = $false
+            $ruleConfiguration.'IgnoreOneLineBlock' = $false
+            $ruleConfiguration.'NewLineAfter' = $false
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
         }
 
@@ -39,6 +48,9 @@ function foo {
 
 }
 '@
+            $ruleConfiguration.'NoEmptyLineBefore' = $true
+            $ruleConfiguration.'IgnoreOneLineBlock' = $false
+            $ruleConfiguration.'NewLineAfter' = $false
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
         }
 
@@ -56,6 +68,9 @@ function foo {
             $def = @'
 $hashtable = @{a = 1; b = 2}
 '@
+            $ruleConfiguration.'NoEmptyLineBefore' = $false
+            $ruleConfiguration.'IgnoreOneLineBlock' = $true
+            $ruleConfiguration.'NewLineAfter' = $false
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
         }
 
@@ -71,6 +86,9 @@ $hashtable = @{
     a = 1
     b = 2}
 '@
+            $ruleConfiguration.'NoEmptyLineBefore' = $false
+            $ruleConfiguration.'IgnoreOneLineBlock' = $true
+            $ruleConfiguration.'NewLineAfter' = $false
             $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
         }
 
@@ -101,4 +119,41 @@ $x = if ($true) { "blah" } else { "blah blah" }
             $violations.Count | Should Be 0
         }
     }
+
+    Context "When a close brace should be follow a new line" {
+        BeforeAll {
+            $def = @'
+if (Test-Path "blah") {
+    "blah"
+} else {
+    "blah blah"
+}
+'@
+            $ruleConfiguration.'NoEmptyLineBefore' = $false
+            $ruleConfiguration.'IgnoreOneLineBlock' = $false
+            $ruleConfiguration.'NewLineAfter' = $true
+            $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+        }
+
+        It "Should find two violations" {
+            $violations.Count | Should Be 2
+            $params = @{
+                RawContent = $def
+                DiagnosticRecord = $violations[0]
+                CorrectionsCount = 1
+                ViolationText = '}'
+                CorrectionText = '}' + [System.Environment]::NewLine
+            }
+            Test-CorrectionExtentFromContent @params
+
+            $params = @{
+                RawContent = $def
+                DiagnosticRecord = $violations[1]
+                CorrectionsCount = 1
+                ViolationText = '}'
+                CorrectionText = '}' + [System.Environment]::NewLine
+            }
+            Test-CorrectionExtentFromContent @params
+        }
+    }
 }