Added rule: AvoidLongLines (#1329)

* Added rule AvoidLongLines * cleaning up typos * Made line length configurable and disabled rule by default a/p @bergmeister suggestion * Updated tests for AvoidLongLines * Fix test failures due to added rule and missing entry in main README.md of rule docs * Add documentation * Added rule AvoidLongLines * cleaning up typos * Made line length configurable and disabled rule by default a/p @bergmeister suggestion * Updated tests for AvoidLongLines * Fix test failures due to added rule and missing entry in main README.md of rule docs * Add documentation * changes a/p feedback from rjmholt * Added test to ensure the rule gets the right extent
PowerShell · Oct 4, 2019 · 2260653 · 2260653
1 parent e37daeb
commit 2260653
Show file tree

Hide file tree

Showing 7 changed files with 316 additions and 1 deletion.
diff --git a/RuleDocumentation/AvoidLongLines.md b/RuleDocumentation/AvoidLongLines.md
@@ -0,0 +1,30 @@
+# AvoidLongLines
+
+**Severity Level: Warning**
+
+## Description
+
+Lines should be no longer than a configured number of characters (default: 120), including leading whitespace (indentation).
+
+**Note**: This rule is not enabled by default. The user needs to enable it through settings.
+
+## Configuration
+
+```powershell
+    Rules = @{
+        PSAvoidLongLines  = @{
+            Enable     = $true
+            LineLength = 120
+        }
+    }
+```
+
+### Parameters
+
+#### Enable: bool (Default value is `$false`)
+
+Enable or disable the rule during ScriptAnalyzer invocation.
+
+#### MaximumLineLength: int (Default value is 120)
+
+Optional parameter to override the default maximum line length.
diff --git a/RuleDocumentation/README.md b/RuleDocumentation/README.md
@@ -12,6 +12,7 @@
 |[AvoidGlobalFunctions](./AvoidGlobalFunctions.md) | Warning | |
 |[AvoidGlobalVars](./AvoidGlobalVars.md) | Warning | |
 |[AvoidInvokingEmptyMembers](./AvoidInvokingEmptyMembers.md) | Warning | |
+|[AvoidLongLines](./AvoidLongLines.md) | Warning | |
 |[AvoidNullOrEmptyHelpMessageAttribute](./AvoidNullOrEmptyHelpMessageAttribute.md) | Warning | |
 |[AvoidShouldContinueWithoutForce](./AvoidShouldContinueWithoutForce.md) | Warning | |
 |[AvoidUsingCmdletAliases](./AvoidUsingCmdletAliases.md) | Warning | Yes |

diff --git a/Rules/AvoidLongLines.cs b/Rules/AvoidLongLines.cs
@@ -0,0 +1,156 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Collections.Generic;
+using System.Text.RegularExpressions;
+#if !CORECLR
+using System.ComponentModel.Composition;
+#endif
+using System.Globalization;
+using System.Management.Automation.Language;
+using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic;
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
+{
+    /// <summary>
+    /// AvoidLongLines: Checks for lines longer than 120 characters
+    /// </summary>
+#if !CORECLR
+    [Export(typeof(IScriptRule))]
+#endif
+    public class AvoidLongLines : ConfigurableRule
+    {
+        /// <summary>
+        /// Construct an object of AvoidLongLines type.
+        /// </summary>
+        public AvoidLongLines()
+        { }
+
+        [ConfigurableRuleProperty(defaultValue: 120)]
+        public int MaximumLineLength { get; set; }
+
+        private readonly string[] s_lineSeparators = new[] { "\r\n", "\n" };
+
+        /// <summary>
+        /// Analyzes the given ast to find violations.
+        /// </summary>
+        /// <param name="ast">AST to be analyzed. This should be non-null</param>
+        /// <param name="fileName">Name of file that corresponds to the input AST.</param>
+        /// <returns>A an enumerable type containing the violations</returns>
+        public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string fileName)
+        {
+            if (ast == null)
+            {
+                throw new ArgumentNullException(nameof(ast));
+            }
+
+            var diagnosticRecords = new List<DiagnosticRecord>();
+
+            string[] lines = ast.Extent.Text.Split(s_lineSeparators, StringSplitOptions.None);
+
+            for (int lineNumber = 0; lineNumber < lines.Length; lineNumber++)
+            {
+                string line = lines[lineNumber];
+
+                if (line.Length <= MaximumLineLength)
+                {
+                    continue;
+                }
+
+                int startLine = lineNumber + 1;
+                int endLine = startLine;
+                int startColumn = 1;
+                int endColumn = line.Length;
+
+                var violationExtent = new ScriptExtent(
+                    new ScriptPosition(
+                        ast.Extent.File,
+                        startLine,
+                        startColumn,
+                        line
+                    ),
+                    new ScriptPosition(
+                        ast.Extent.File,
+                        endLine,
+                        endColumn,
+                        line
+                    ));
+
+                var record = new DiagnosticRecord(
+                        String.Format(CultureInfo.CurrentCulture, 
+                            String.Format(Strings.AvoidLongLinesError, MaximumLineLength)),
+                        violationExtent,
+                        GetName(),
+                        GetDiagnosticSeverity(),
+                        ast.Extent.File,
+                        null
+                    );
+                diagnosticRecords.Add(record);
+            }
+
+            return diagnosticRecords;
+        }
+
+        /// <summary>
+        /// Retrieves the common name of this rule.
+        /// </summary>
+        public override string GetCommonName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesCommonName);
+        }
+
+        /// <summary>
+        /// Retrieves the description of this rule.
+        /// </summary>
+        public override string GetDescription()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesDescription);
+        }
+
+        /// <summary>
+        /// Retrieves the name of this rule.
+        /// </summary>
+        public override string GetName()
+        {
+            return string.Format(
+                CultureInfo.CurrentCulture,
+                Strings.NameSpaceFormat,
+                GetSourceName(),
+                Strings.AvoidLongLinesName);
+        }
+
+        /// <summary>
+        /// Retrieves the severity of the rule: error, warning or information.
+        /// </summary>
+        public override RuleSeverity GetSeverity()
+        {
+            return RuleSeverity.Warning;
+        }
+
+        /// <summary>
+        /// Gets the severity of the returned diagnostic record: error, warning, or information.
+        /// </summary>
+        /// <returns></returns>
+        public DiagnosticSeverity GetDiagnosticSeverity()
+        {
+            return DiagnosticSeverity.Warning;
+        }
+
+        /// <summary>
+        /// Retrieves the name of the module/assembly the rule is from.
+        /// </summary>
+        public override string GetSourceName()
+        {
+            return string.Format(CultureInfo.CurrentCulture, Strings.SourceName);
+        }
+
+        /// <summary>
+        /// Retrieves the type of the rule, Builtin, Managed or Module.
+        /// </summary>
+        public override SourceType GetSourceType()
+        {
+            return SourceType.Builtin;
+        }
+    }
+}
diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs
diff --git a/Rules/Strings.resx b/Rules/Strings.resx
@@ -897,6 +897,18 @@
   <data name="AvoidTrailingWhitespaceError" xml:space="preserve">
     <value>Line has trailing whitespace</value>
   </data>
+  <data name="AvoidLongLinesName" xml:space="preserve">
+    <value>AvoidLongLines</value>
+  </data>
+  <data name="AvoidLongLinesCommonName" xml:space="preserve">
+    <value>Avoid long lines</value>
+  </data>
+  <data name="AvoidLongLinesDescription" xml:space="preserve">
+    <value>Line lengths should be less than the configured maximum</value>
+  </data>
+  <data name="AvoidLongLinesError" xml:space="preserve">
+    <value>Line exceeds the configured maximum length of {0} characters</value>
+  </data>
   <data name="PlaceOpenBraceName" xml:space="preserve">
     <value>PlaceOpenBrace</value>
   </data>

diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1
@@ -59,7 +59,7 @@ Describe "Test Name parameters" {
 
         It "get Rules with no parameters supplied" {
 			$defaultRules = Get-ScriptAnalyzerRule
-            $expectedNumRules = 59
+            $expectedNumRules = 60
             if ((Test-PSEditionCoreClr) -or (Test-PSVersionV3) -or (Test-PSVersionV4))
             {
                 # for PSv3 PSAvoidGlobalAliases is not shipped because

diff --git a/Tests/Rules/AvoidLongLines.tests.ps1 b/Tests/Rules/AvoidLongLines.tests.ps1
@@ -0,0 +1,61 @@
+$ruleName = "PSAvoidLongLines"
+
+$ruleSettings = @{
+    Enable = $true
+}
+$settings = @{
+    IncludeRules = @($ruleName)
+    Rules        = @{ $ruleName = $ruleSettings }
+}
+
+Describe "AvoidLongLines" {
+    it 'Should be off by default' {
+        $def = "a" * 500
+        $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def
+        $violations.Count | Should -Be 0
+    }
+
+    it 'Should find a violation when a line is longer than 120 characters (no whitespace)' {
+        $def = @"
+$("a" * 500)
+this line is short enough
+"@
+        $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+        $violations.Count | Should -Be 1
+    }
+
+    it 'Should get the correct extent of the violation' {
+        $def = @"
+this line is short enough
+$("a" * 500)
+"@
+        $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+        $violations[0].Extent.StartLineNumber | Should -Be 2
+        $violations[0].Extent.EndLineNumber | Should -Be 2
+        $violations[0].Extent.StartColumnNumber | Should -Be 1
+        $violations[0].Extent.EndColumnNumber | Should -Be 500
+    }
+
+    it 'Should find a violation when a line is longer than 120 characters (leading whitespace)' {
+        $def = @"
+$(" " * 100 + "a" * 25)
+this line is short enough
+"@
+        $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+        $violations.Count | Should -Be 1
+    }
+
+    it 'Should not find a violation for lines under 120 characters' {
+        $def = "a" * 120
+        $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+        $violations.Count | Should -Be 0
+    }
+
+    it 'Should find a violation with a configured line length' {
+        $ruleSettings.Add('MaximumLineLength', 10)
+        $settings['Rules'] = @{ $ruleName = $ruleSettings }
+        $def = "a" * 15
+        $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
+        $violations.Count | Should -Be 1
+    }
+}