From a356d41e069938ef02312fb2414aadabd8cdc75d Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 3 Sep 2019 11:38:45 -0700 Subject: [PATCH 01/14] Added rule AvoidLongLines --- RuleDocumentation/AvoidLongLines.md | 7 ++ Rules/AvoidLongLines.cs | 142 +++++++++++++++++++++++++++ Rules/Strings.Designer.cs | 55 +++++++++++ Rules/Strings.resx | 12 +++ Tests/Rules/AvoidLongLines.tests.ps1 | 25 +++++ 5 files changed, 241 insertions(+) create mode 100644 RuleDocumentation/AvoidLongLines.md create mode 100644 Rules/AvoidLongLines.cs create mode 100644 Tests/Rules/AvoidLongLines.tests.ps1 diff --git a/RuleDocumentation/AvoidLongLines.md b/RuleDocumentation/AvoidLongLines.md new file mode 100644 index 000000000..ec08695b9 --- /dev/null +++ b/RuleDocumentation/AvoidLongLines.md @@ -0,0 +1,7 @@ +# AvoidLongLines + +**Severity Level: Warning** + +## Description + +Lines should be longer than 120 characters, including leading whitespace (indentation). diff --git a/Rules/AvoidLongLines.cs b/Rules/AvoidLongLines.cs new file mode 100644 index 000000000..d76a329b5 --- /dev/null +++ b/Rules/AvoidLongLines.cs @@ -0,0 +1,142 @@ +// 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 +{ + /// + /// AvoidTrailingWhitespace: Checks for trailing whitespaces + /// +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + public class AvoidLongLines : IScriptRule + { + /// + /// Analyzes the given ast to find violations. + /// + /// AST to be analyzed. This should be non-null + /// Name of file that corresponds to the input AST. + /// A an enumerable type containing the violations + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) + { + throw new ArgumentNullException("ast"); + } + + var diagnosticRecords = new List(); + + string[] lines = Regex.Split(ast.Extent.Text, @"\r?\n"); + + for (int lineNumber = 0; lineNumber < lines.Length; lineNumber++) + { + var line = lines[lineNumber]; + + if (line.Length > 120) + { + var startLine = lineNumber + 1; + var endLine = startLine; + var startColumn = 1; + var 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, Strings.AvoidLongLinesError), + violationExtent, + GetName(), + GetDiagnosticSeverity(), + ast.Extent.File, + null + ); + diagnosticRecords.Add(record); + } + } + + return diagnosticRecords; + } + + /// + /// Retrieves the common name of this rule. + /// + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesCommonName); + } + + /// + /// Retrieves the description of this rule. + /// + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesDescription); + } + + /// + /// Retrieves the name of this rule. + /// + public string GetName() + { + return string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.AvoidLongLinesName); + } + + /// + /// Retrieves the severity of the rule: error, warning or information. + /// + public RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// Gets the severity of the returned diagnostic record: error, warning, or information. + /// + /// + public DiagnosticSeverity GetDiagnosticSeverity() + { + return DiagnosticSeverity.Warning; + } + + /// + /// Retrieves the name of the module/assembly the rule is from. + /// + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + + /// + /// Retrieves the type of the rule, Builtin, Managed or Module. + /// + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + } +} diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 8c4f6fc5f..42d5a846e 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -536,6 +536,61 @@ internal static string AvoidTrailingWhitespaceName { return ResourceManager.GetString("AvoidTrailingWhitespaceName", resourceCulture); } } + + /// + /// Looks up a localized string similar to AvoidLongLines. + /// + internal static string AvoidLongLinesName + { + get + { + return ResourceManager.GetString("AvoidLongLinesName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Avoid long lines. + /// + internal static string AvoidLongLinesCommonName + { + get + { + return ResourceManager.GetString("AvoidLongLinesCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Each line should be under 120 characters. + /// + internal static string AvoidLongLinesDescription + { + get + { + return ResourceManager.GetString("AvoidLongLinesDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Line is longer than 120 characters. + /// + internal static string AvoidLongLinesError + { + get + { + return ResourceManager.GetString("AvoidLongLinesError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidLongLines. + /// + internal static string AvoidLongLinesWhitespaceName + { + get + { + return ResourceManager.GetString("AvoidLongLinesName", resourceCulture); + } + } /// /// Looks up a localized string similar to Module Must Be Loadable. diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 4f6a29df3..aa0be7577 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -897,6 +897,18 @@ Line has trailing whitespace + + AvoidLongLines + + + Avoid long lines + + + Each line should be no longer than 120 characters. + + + Line is longer than 120 characters which is too long + PlaceOpenBrace diff --git a/Tests/Rules/AvoidLongLines.tests.ps1 b/Tests/Rules/AvoidLongLines.tests.ps1 new file mode 100644 index 000000000..45c19d171 --- /dev/null +++ b/Tests/Rules/AvoidLongLines.tests.ps1 @@ -0,0 +1,25 @@ +$ruleName = "PSAvoidLongLines" + +$settings = @{ + IncludeRules = @($ruleName) +} + +Describe "AvoidLongLines" { + it 'Should find a violation when a line is longer than 120 characters (no whitespace)' { + $def = "a" * 125 + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should -Be 1 + } + + it 'Should find a violation when a line is longer than 120 characters (leading whitespace)' { + $def = " " * 100 + "a" * 25 + $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 + } +} From b6a1c9f5804012e818722566cb2b4faeea825239 Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 3 Sep 2019 12:47:03 -0700 Subject: [PATCH 02/14] cleaning up typos --- RuleDocumentation/AvoidLongLines.md | 2 +- Rules/AvoidLongLines.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/RuleDocumentation/AvoidLongLines.md b/RuleDocumentation/AvoidLongLines.md index ec08695b9..d8103faa3 100644 --- a/RuleDocumentation/AvoidLongLines.md +++ b/RuleDocumentation/AvoidLongLines.md @@ -4,4 +4,4 @@ ## Description -Lines should be longer than 120 characters, including leading whitespace (indentation). +Lines should be no longer than 120 characters, including leading whitespace (indentation). diff --git a/Rules/AvoidLongLines.cs b/Rules/AvoidLongLines.cs index d76a329b5..4a7be4558 100644 --- a/Rules/AvoidLongLines.cs +++ b/Rules/AvoidLongLines.cs @@ -14,7 +14,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { /// - /// AvoidTrailingWhitespace: Checks for trailing whitespaces + /// AvoidLongLines: Checks for lines longer than 120 characters /// #if !CORECLR [Export(typeof(IScriptRule))] From 71acba426302afdc4a9cedac6a4d37bc2bcf2da5 Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 3 Sep 2019 13:50:01 -0700 Subject: [PATCH 03/14] Made line length configurable and disabled rule by default a/p @bergmeister suggestion --- Rules/AvoidLongLines.cs | 29 ++++++++++++++++++++--------- Rules/Strings.resx | 4 ++-- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/Rules/AvoidLongLines.cs b/Rules/AvoidLongLines.cs index 4a7be4558..f8ed2ea4c 100644 --- a/Rules/AvoidLongLines.cs +++ b/Rules/AvoidLongLines.cs @@ -19,15 +19,26 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #if !CORECLR [Export(typeof(IScriptRule))] #endif - public class AvoidLongLines : IScriptRule + public class AvoidLongLines : ConfigurableRule { + /// + /// Construct an object of AvoidLongLines type. + /// + public AvoidLongLines() : base() + { + // Enable the rule by default + Enable = false; + } + + [ConfigurableRuleProperty(defaultValue: 120)] + public int LineLength { get; set; } /// /// Analyzes the given ast to find violations. /// /// AST to be analyzed. This should be non-null /// Name of file that corresponds to the input AST. /// A an enumerable type containing the violations - public IEnumerable AnalyzeScript(Ast ast, string fileName) + public override IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) { @@ -42,7 +53,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { var line = lines[lineNumber]; - if (line.Length > 120) + if (line.Length > LineLength) { var startLine = lineNumber + 1; var endLine = startLine; @@ -81,7 +92,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// /// Retrieves the common name of this rule. /// - public string GetCommonName() + public override string GetCommonName() { return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesCommonName); } @@ -89,7 +100,7 @@ public string GetCommonName() /// /// Retrieves the description of this rule. /// - public string GetDescription() + public override string GetDescription() { return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesDescription); } @@ -97,7 +108,7 @@ public string GetDescription() /// /// Retrieves the name of this rule. /// - public string GetName() + public override string GetName() { return string.Format( CultureInfo.CurrentCulture, @@ -109,7 +120,7 @@ public string GetName() /// /// Retrieves the severity of the rule: error, warning or information. /// - public RuleSeverity GetSeverity() + public override RuleSeverity GetSeverity() { return RuleSeverity.Warning; } @@ -126,7 +137,7 @@ public DiagnosticSeverity GetDiagnosticSeverity() /// /// Retrieves the name of the module/assembly the rule is from. /// - public string GetSourceName() + public override string GetSourceName() { return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); } @@ -134,7 +145,7 @@ public string GetSourceName() /// /// Retrieves the type of the rule, Builtin, Managed or Module. /// - public SourceType GetSourceType() + public override SourceType GetSourceType() { return SourceType.Builtin; } diff --git a/Rules/Strings.resx b/Rules/Strings.resx index aa0be7577..bf231cce3 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -904,10 +904,10 @@ Avoid long lines - Each line should be no longer than 120 characters. + Each line should be not be too long. - Line is longer than 120 characters which is too long + Line is too long PlaceOpenBrace From aa3b56d358d2b3b49a2a274d73d68d8a730564ab Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 3 Sep 2019 14:07:29 -0700 Subject: [PATCH 04/14] Updated tests for AvoidLongLines --- Tests/Rules/AvoidLongLines.tests.ps1 | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/Tests/Rules/AvoidLongLines.tests.ps1 b/Tests/Rules/AvoidLongLines.tests.ps1 index 45c19d171..4086102ac 100644 --- a/Tests/Rules/AvoidLongLines.tests.ps1 +++ b/Tests/Rules/AvoidLongLines.tests.ps1 @@ -1,10 +1,20 @@ $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" * 125 $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings @@ -22,4 +32,12 @@ Describe "AvoidLongLines" { $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings $violations.Count | Should -Be 0 } + + it 'Should find a violation with a configured line length' { + $ruleSettings.Add('LineLength', 10) + $settings['Rules'] = @{ $ruleName = $ruleSettings } + $def = "a" * 15 + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should -Be 1 + } } From 4683cd874f4f4b8b88bda6d71bb4a8daff888cd4 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 15 Sep 2019 12:39:40 +0100 Subject: [PATCH 05/14] Fix test failures due to added rule and missing entry in main README.md of rule docs --- RuleDocumentation/README.md | 1 + Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/RuleDocumentation/README.md b/RuleDocumentation/README.md index 02f56a394..e9d3f66ce 100644 --- 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/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 668590672..8b68c63f3 100644 --- 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 From df0eec23a8677fd76e6613a81a7f5930176b6715 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 15 Sep 2019 17:32:27 +0100 Subject: [PATCH 06/14] Add documentation --- RuleDocumentation/AvoidLongLines.md | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/RuleDocumentation/AvoidLongLines.md b/RuleDocumentation/AvoidLongLines.md index d8103faa3..c20c48923 100644 --- a/RuleDocumentation/AvoidLongLines.md +++ b/RuleDocumentation/AvoidLongLines.md @@ -4,4 +4,27 @@ ## Description -Lines should be no longer than 120 characters, including leading whitespace (indentation). +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. + +#### LineLength: int (Default value is 120) + +Optional parameter do override the default line length. From ac249ee9537ae7eb79f5630a638c8cefdffd0a9d Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 3 Sep 2019 11:38:45 -0700 Subject: [PATCH 07/14] Added rule AvoidLongLines --- RuleDocumentation/AvoidLongLines.md | 7 ++ Rules/AvoidLongLines.cs | 142 +++++++++++++++++++++++++++ Rules/Strings.Designer.cs | 55 +++++++++++ Rules/Strings.resx | 12 +++ Tests/Rules/AvoidLongLines.tests.ps1 | 25 +++++ 5 files changed, 241 insertions(+) create mode 100644 RuleDocumentation/AvoidLongLines.md create mode 100644 Rules/AvoidLongLines.cs create mode 100644 Tests/Rules/AvoidLongLines.tests.ps1 diff --git a/RuleDocumentation/AvoidLongLines.md b/RuleDocumentation/AvoidLongLines.md new file mode 100644 index 000000000..ec08695b9 --- /dev/null +++ b/RuleDocumentation/AvoidLongLines.md @@ -0,0 +1,7 @@ +# AvoidLongLines + +**Severity Level: Warning** + +## Description + +Lines should be longer than 120 characters, including leading whitespace (indentation). diff --git a/Rules/AvoidLongLines.cs b/Rules/AvoidLongLines.cs new file mode 100644 index 000000000..d76a329b5 --- /dev/null +++ b/Rules/AvoidLongLines.cs @@ -0,0 +1,142 @@ +// 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 +{ + /// + /// AvoidTrailingWhitespace: Checks for trailing whitespaces + /// +#if !CORECLR + [Export(typeof(IScriptRule))] +#endif + public class AvoidLongLines : IScriptRule + { + /// + /// Analyzes the given ast to find violations. + /// + /// AST to be analyzed. This should be non-null + /// Name of file that corresponds to the input AST. + /// A an enumerable type containing the violations + public IEnumerable AnalyzeScript(Ast ast, string fileName) + { + if (ast == null) + { + throw new ArgumentNullException("ast"); + } + + var diagnosticRecords = new List(); + + string[] lines = Regex.Split(ast.Extent.Text, @"\r?\n"); + + for (int lineNumber = 0; lineNumber < lines.Length; lineNumber++) + { + var line = lines[lineNumber]; + + if (line.Length > 120) + { + var startLine = lineNumber + 1; + var endLine = startLine; + var startColumn = 1; + var 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, Strings.AvoidLongLinesError), + violationExtent, + GetName(), + GetDiagnosticSeverity(), + ast.Extent.File, + null + ); + diagnosticRecords.Add(record); + } + } + + return diagnosticRecords; + } + + /// + /// Retrieves the common name of this rule. + /// + public string GetCommonName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesCommonName); + } + + /// + /// Retrieves the description of this rule. + /// + public string GetDescription() + { + return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesDescription); + } + + /// + /// Retrieves the name of this rule. + /// + public string GetName() + { + return string.Format( + CultureInfo.CurrentCulture, + Strings.NameSpaceFormat, + GetSourceName(), + Strings.AvoidLongLinesName); + } + + /// + /// Retrieves the severity of the rule: error, warning or information. + /// + public RuleSeverity GetSeverity() + { + return RuleSeverity.Warning; + } + + /// + /// Gets the severity of the returned diagnostic record: error, warning, or information. + /// + /// + public DiagnosticSeverity GetDiagnosticSeverity() + { + return DiagnosticSeverity.Warning; + } + + /// + /// Retrieves the name of the module/assembly the rule is from. + /// + public string GetSourceName() + { + return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); + } + + /// + /// Retrieves the type of the rule, Builtin, Managed or Module. + /// + public SourceType GetSourceType() + { + return SourceType.Builtin; + } + } +} diff --git a/Rules/Strings.Designer.cs b/Rules/Strings.Designer.cs index 8c4f6fc5f..42d5a846e 100644 --- a/Rules/Strings.Designer.cs +++ b/Rules/Strings.Designer.cs @@ -536,6 +536,61 @@ internal static string AvoidTrailingWhitespaceName { return ResourceManager.GetString("AvoidTrailingWhitespaceName", resourceCulture); } } + + /// + /// Looks up a localized string similar to AvoidLongLines. + /// + internal static string AvoidLongLinesName + { + get + { + return ResourceManager.GetString("AvoidLongLinesName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Avoid long lines. + /// + internal static string AvoidLongLinesCommonName + { + get + { + return ResourceManager.GetString("AvoidLongLinesCommonName", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Each line should be under 120 characters. + /// + internal static string AvoidLongLinesDescription + { + get + { + return ResourceManager.GetString("AvoidLongLinesDescription", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to Line is longer than 120 characters. + /// + internal static string AvoidLongLinesError + { + get + { + return ResourceManager.GetString("AvoidLongLinesError", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to AvoidLongLines. + /// + internal static string AvoidLongLinesWhitespaceName + { + get + { + return ResourceManager.GetString("AvoidLongLinesName", resourceCulture); + } + } /// /// Looks up a localized string similar to Module Must Be Loadable. diff --git a/Rules/Strings.resx b/Rules/Strings.resx index 4f6a29df3..aa0be7577 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -897,6 +897,18 @@ Line has trailing whitespace + + AvoidLongLines + + + Avoid long lines + + + Each line should be no longer than 120 characters. + + + Line is longer than 120 characters which is too long + PlaceOpenBrace diff --git a/Tests/Rules/AvoidLongLines.tests.ps1 b/Tests/Rules/AvoidLongLines.tests.ps1 new file mode 100644 index 000000000..45c19d171 --- /dev/null +++ b/Tests/Rules/AvoidLongLines.tests.ps1 @@ -0,0 +1,25 @@ +$ruleName = "PSAvoidLongLines" + +$settings = @{ + IncludeRules = @($ruleName) +} + +Describe "AvoidLongLines" { + it 'Should find a violation when a line is longer than 120 characters (no whitespace)' { + $def = "a" * 125 + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should -Be 1 + } + + it 'Should find a violation when a line is longer than 120 characters (leading whitespace)' { + $def = " " * 100 + "a" * 25 + $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 + } +} From 2737d6ed3a5d3ae51bf85659653a82ff1f68fb20 Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 3 Sep 2019 12:47:03 -0700 Subject: [PATCH 08/14] cleaning up typos --- RuleDocumentation/AvoidLongLines.md | 2 +- Rules/AvoidLongLines.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/RuleDocumentation/AvoidLongLines.md b/RuleDocumentation/AvoidLongLines.md index ec08695b9..d8103faa3 100644 --- a/RuleDocumentation/AvoidLongLines.md +++ b/RuleDocumentation/AvoidLongLines.md @@ -4,4 +4,4 @@ ## Description -Lines should be longer than 120 characters, including leading whitespace (indentation). +Lines should be no longer than 120 characters, including leading whitespace (indentation). diff --git a/Rules/AvoidLongLines.cs b/Rules/AvoidLongLines.cs index d76a329b5..4a7be4558 100644 --- a/Rules/AvoidLongLines.cs +++ b/Rules/AvoidLongLines.cs @@ -14,7 +14,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { /// - /// AvoidTrailingWhitespace: Checks for trailing whitespaces + /// AvoidLongLines: Checks for lines longer than 120 characters /// #if !CORECLR [Export(typeof(IScriptRule))] From 7a40b9df12c30c5ae0f09a106629f4677470b784 Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 3 Sep 2019 13:50:01 -0700 Subject: [PATCH 09/14] Made line length configurable and disabled rule by default a/p @bergmeister suggestion --- Rules/AvoidLongLines.cs | 29 ++++++++++++++++++++--------- Rules/Strings.resx | 4 ++-- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/Rules/AvoidLongLines.cs b/Rules/AvoidLongLines.cs index 4a7be4558..f8ed2ea4c 100644 --- a/Rules/AvoidLongLines.cs +++ b/Rules/AvoidLongLines.cs @@ -19,15 +19,26 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules #if !CORECLR [Export(typeof(IScriptRule))] #endif - public class AvoidLongLines : IScriptRule + public class AvoidLongLines : ConfigurableRule { + /// + /// Construct an object of AvoidLongLines type. + /// + public AvoidLongLines() : base() + { + // Enable the rule by default + Enable = false; + } + + [ConfigurableRuleProperty(defaultValue: 120)] + public int LineLength { get; set; } /// /// Analyzes the given ast to find violations. /// /// AST to be analyzed. This should be non-null /// Name of file that corresponds to the input AST. /// A an enumerable type containing the violations - public IEnumerable AnalyzeScript(Ast ast, string fileName) + public override IEnumerable AnalyzeScript(Ast ast, string fileName) { if (ast == null) { @@ -42,7 +53,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) { var line = lines[lineNumber]; - if (line.Length > 120) + if (line.Length > LineLength) { var startLine = lineNumber + 1; var endLine = startLine; @@ -81,7 +92,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// /// Retrieves the common name of this rule. /// - public string GetCommonName() + public override string GetCommonName() { return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesCommonName); } @@ -89,7 +100,7 @@ public string GetCommonName() /// /// Retrieves the description of this rule. /// - public string GetDescription() + public override string GetDescription() { return string.Format(CultureInfo.CurrentCulture, Strings.AvoidLongLinesDescription); } @@ -97,7 +108,7 @@ public string GetDescription() /// /// Retrieves the name of this rule. /// - public string GetName() + public override string GetName() { return string.Format( CultureInfo.CurrentCulture, @@ -109,7 +120,7 @@ public string GetName() /// /// Retrieves the severity of the rule: error, warning or information. /// - public RuleSeverity GetSeverity() + public override RuleSeverity GetSeverity() { return RuleSeverity.Warning; } @@ -126,7 +137,7 @@ public DiagnosticSeverity GetDiagnosticSeverity() /// /// Retrieves the name of the module/assembly the rule is from. /// - public string GetSourceName() + public override string GetSourceName() { return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); } @@ -134,7 +145,7 @@ public string GetSourceName() /// /// Retrieves the type of the rule, Builtin, Managed or Module. /// - public SourceType GetSourceType() + public override SourceType GetSourceType() { return SourceType.Builtin; } diff --git a/Rules/Strings.resx b/Rules/Strings.resx index aa0be7577..bf231cce3 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -904,10 +904,10 @@ Avoid long lines - Each line should be no longer than 120 characters. + Each line should be not be too long. - Line is longer than 120 characters which is too long + Line is too long PlaceOpenBrace From 7af723402cf6ba8183152e5e7b53efa049fcf192 Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 3 Sep 2019 14:07:29 -0700 Subject: [PATCH 10/14] Updated tests for AvoidLongLines --- Tests/Rules/AvoidLongLines.tests.ps1 | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/Tests/Rules/AvoidLongLines.tests.ps1 b/Tests/Rules/AvoidLongLines.tests.ps1 index 45c19d171..4086102ac 100644 --- a/Tests/Rules/AvoidLongLines.tests.ps1 +++ b/Tests/Rules/AvoidLongLines.tests.ps1 @@ -1,10 +1,20 @@ $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" * 125 $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings @@ -22,4 +32,12 @@ Describe "AvoidLongLines" { $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings $violations.Count | Should -Be 0 } + + it 'Should find a violation with a configured line length' { + $ruleSettings.Add('LineLength', 10) + $settings['Rules'] = @{ $ruleName = $ruleSettings } + $def = "a" * 15 + $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings + $violations.Count | Should -Be 1 + } } From 165c180c5342a53e05e4594ef17bed407ab024dc Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 15 Sep 2019 12:39:40 +0100 Subject: [PATCH 11/14] Fix test failures due to added rule and missing entry in main README.md of rule docs --- RuleDocumentation/README.md | 1 + Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/RuleDocumentation/README.md b/RuleDocumentation/README.md index 02f56a394..e9d3f66ce 100644 --- 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/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 668590672..8b68c63f3 100644 --- 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 From 6501709e29c5a845df58e5bd937ed0315d1427f9 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 15 Sep 2019 17:32:27 +0100 Subject: [PATCH 12/14] Add documentation --- RuleDocumentation/AvoidLongLines.md | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/RuleDocumentation/AvoidLongLines.md b/RuleDocumentation/AvoidLongLines.md index d8103faa3..c20c48923 100644 --- a/RuleDocumentation/AvoidLongLines.md +++ b/RuleDocumentation/AvoidLongLines.md @@ -4,4 +4,27 @@ ## Description -Lines should be no longer than 120 characters, including leading whitespace (indentation). +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. + +#### LineLength: int (Default value is 120) + +Optional parameter do override the default line length. From 8179dbe0ebd675e6c62db3def808f9f4b91efab5 Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 17 Sep 2019 09:04:28 -0700 Subject: [PATCH 13/14] changes a/p feedback from rjmholt --- RuleDocumentation/AvoidLongLines.md | 4 +- Rules/AvoidLongLines.cs | 79 +++++++++++++++------------- Rules/Strings.resx | 4 +- Tests/Rules/AvoidLongLines.tests.ps1 | 12 +++-- 4 files changed, 54 insertions(+), 45 deletions(-) diff --git a/RuleDocumentation/AvoidLongLines.md b/RuleDocumentation/AvoidLongLines.md index c20c48923..0ea2c9a65 100644 --- a/RuleDocumentation/AvoidLongLines.md +++ b/RuleDocumentation/AvoidLongLines.md @@ -25,6 +25,6 @@ Lines should be no longer than a configured number of characters (default: 120), Enable or disable the rule during ScriptAnalyzer invocation. -#### LineLength: int (Default value is 120) +#### MaximumLineLength: int (Default value is 120) -Optional parameter do override the default line length. +Optional parameter to override the default maximum line length. diff --git a/Rules/AvoidLongLines.cs b/Rules/AvoidLongLines.cs index f8ed2ea4c..e0860a75e 100644 --- a/Rules/AvoidLongLines.cs +++ b/Rules/AvoidLongLines.cs @@ -24,14 +24,14 @@ public class AvoidLongLines : ConfigurableRule /// /// Construct an object of AvoidLongLines type. /// - public AvoidLongLines() : base() - { - // Enable the rule by default - Enable = false; - } + public AvoidLongLines() + { } [ConfigurableRuleProperty(defaultValue: 120)] - public int LineLength { get; set; } + public int MaximumLineLength { get; set; } + + private readonly string[] s_lineSeparators = new[] { "\r\n", "\n" }; + /// /// Analyzes the given ast to find violations. /// @@ -42,48 +42,51 @@ public override IEnumerable AnalyzeScript(Ast ast, string file { if (ast == null) { - throw new ArgumentNullException("ast"); + throw new ArgumentNullException(nameof(ast)); } var diagnosticRecords = new List(); - string[] lines = Regex.Split(ast.Extent.Text, @"\r?\n"); + string[] lines = ast.Extent.Text.Split(s_lineSeparators, StringSplitOptions.None); for (int lineNumber = 0; lineNumber < lines.Length; lineNumber++) { - var line = lines[lineNumber]; + string line = lines[lineNumber]; - if (line.Length > LineLength) + if (line.Length <= MaximumLineLength) { - var startLine = lineNumber + 1; - var endLine = startLine; - var startColumn = 1; - var 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, Strings.AvoidLongLinesError), - violationExtent, - GetName(), - GetDiagnosticSeverity(), - ast.Extent.File, - null - ); - diagnosticRecords.Add(record); + 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; diff --git a/Rules/Strings.resx b/Rules/Strings.resx index bf231cce3..dd5825e53 100644 --- a/Rules/Strings.resx +++ b/Rules/Strings.resx @@ -904,10 +904,10 @@ Avoid long lines - Each line should be not be too long. + Line lengths should be less than the configured maximum - Line is too long + Line exceeds the configured maximum length of {0} characters PlaceOpenBrace diff --git a/Tests/Rules/AvoidLongLines.tests.ps1 b/Tests/Rules/AvoidLongLines.tests.ps1 index 4086102ac..d627142c8 100644 --- a/Tests/Rules/AvoidLongLines.tests.ps1 +++ b/Tests/Rules/AvoidLongLines.tests.ps1 @@ -16,13 +16,19 @@ Describe "AvoidLongLines" { } it 'Should find a violation when a line is longer than 120 characters (no whitespace)' { - $def = "a" * 125 + $def = @" +$("a" * 500) +this line is short enough +"@ $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings $violations.Count | Should -Be 1 } it 'Should find a violation when a line is longer than 120 characters (leading whitespace)' { - $def = " " * 100 + "a" * 25 + $def = @" +$(" " * 100 + "a" * 25) +this line is short enough +"@ $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings $violations.Count | Should -Be 1 } @@ -34,7 +40,7 @@ Describe "AvoidLongLines" { } it 'Should find a violation with a configured line length' { - $ruleSettings.Add('LineLength', 10) + $ruleSettings.Add('MaximumLineLength', 10) $settings['Rules'] = @{ $ruleName = $ruleSettings } $def = "a" * 15 $violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings From 567d4bcb5eb66e5046252bfd0420222870262b2e Mon Sep 17 00:00:00 2001 From: Thomas Rayner Date: Tue, 24 Sep 2019 10:15:38 -0700 Subject: [PATCH 14/14] Added test to ensure the rule gets the right extent --- Tests/Rules/AvoidLongLines.tests.ps1 | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/Tests/Rules/AvoidLongLines.tests.ps1 b/Tests/Rules/AvoidLongLines.tests.ps1 index d627142c8..5a6c8e189 100644 --- a/Tests/Rules/AvoidLongLines.tests.ps1 +++ b/Tests/Rules/AvoidLongLines.tests.ps1 @@ -5,7 +5,7 @@ $ruleSettings = @{ } $settings = @{ IncludeRules = @($ruleName) - Rules = @{ $ruleName = $ruleSettings } + Rules = @{ $ruleName = $ruleSettings } } Describe "AvoidLongLines" { @@ -24,6 +24,18 @@ this line is short enough $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)