From 77ff6140a78c4e984c2a6d0846ed7ed001598624 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Wed, 30 Aug 2017 22:53:47 +0100 Subject: [PATCH 01/29] Add AutoFix switch parameter for 'File' parameter set, which uses the SuggestedCorrectionExtent information as a correction. It uses UTF8 to read/write the file for avoiding problems with special characters such as the copyright symbol in psd1 files but this probably needs to be enhanced to preserver the encoding of the original file. This is a a minimum (and hopefully) viable implementation of issue 802. It works for most files of the PowerShell repo but throws a 'EditableTextInvalidLineEnding' error for one file (this seems to be an issue with existing code that fixes EditableText) --- Engine/Commands/InvokeScriptAnalyzerCommand.cs | 13 ++++++++++++- Engine/ScriptAnalyzer.cs | 11 ++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index ad25c5e0e..7aebc2e18 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -180,6 +180,17 @@ public SwitchParameter SuppressedOnly } private bool suppressedOnly; + /// + /// Resolves rule violations automatically where possible. + /// + [Parameter(Mandatory = false, ParameterSetName = "File")] + public SwitchParameter AutoFix + { + get { return autoFix; } + set { autoFix = value; } + } + private bool autoFix; + /// /// Returns path to the file that contains user profile or hash table for ScriptAnalyzer /// @@ -377,7 +388,7 @@ private void ProcessInput() { foreach (var p in processedPaths) { - diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.recurse); + diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.recurse, this.autoFix); WriteToOutput(diagnosticsList); } } diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs index 5c3a6ec03..b61e90c4a 100644 --- a/Engine/ScriptAnalyzer.cs +++ b/Engine/ScriptAnalyzer.cs @@ -31,6 +31,7 @@ using System.Collections.ObjectModel; using System.Collections; using System.Diagnostics; +using System.Text; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -1451,11 +1452,12 @@ public Dictionary> CheckRuleExtension(string[] path, PathIn /// /// The path of the file or directory to analyze. /// + /// /// If true, recursively searches the given file path and analyzes any /// script files that are found. /// /// An enumeration of DiagnosticRecords that were found by rules. - public IEnumerable AnalyzePath(string path, bool searchRecursively = false) + public IEnumerable AnalyzePath(string path, bool searchRecursively = false, bool autoFix = false) { List scriptFilePaths = new List(); @@ -1475,6 +1477,13 @@ public IEnumerable AnalyzePath(string path, bool searchRecursi this.BuildScriptPathList(path, searchRecursively, scriptFilePaths); foreach (string scriptFilePath in scriptFilePaths) { + if (autoFix) + { + var scriptFileContentWithAutoFixes = Fix(File.ReadAllText(scriptFilePath)); + // Use UTF8 when writing to avoid issues with special characters such as e.g. the copyright symbol in *.psd1 files. This could be improved to detect the encoding in order to preserve it. + File.WriteAllText(scriptFilePath, scriptFileContentWithAutoFixes, Encoding.UTF8); + } + // Yield each record in the result so that the // caller can pull them one at a time foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) From ef75719c744d8edbbe5e68445138e2cbd31b3cee Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 12 Sep 2017 23:22:37 +0100 Subject: [PATCH 02/29] PR 817: Name switch 'Fix' and preserver Encoding of the file. However in the case of UTF8, a BOM will get added. Update help markdown to fix help related tests. --- .../Commands/InvokeScriptAnalyzerCommand.cs | 10 ++++---- Engine/ScriptAnalyzer.cs | 24 ++++++++++++++----- docs/markdown/Invoke-ScriptAnalyzer.md | 18 +++++++++++++- 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index 7aebc2e18..a393fa1a7 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -184,12 +184,12 @@ public SwitchParameter SuppressedOnly /// Resolves rule violations automatically where possible. /// [Parameter(Mandatory = false, ParameterSetName = "File")] - public SwitchParameter AutoFix + public SwitchParameter Fix { - get { return autoFix; } - set { autoFix = value; } + get { return fix; } + set { fix = value; } } - private bool autoFix; + private bool fix; /// /// Returns path to the file that contains user profile or hash table for ScriptAnalyzer @@ -388,7 +388,7 @@ private void ProcessInput() { foreach (var p in processedPaths) { - diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.recurse, this.autoFix); + diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.recurse, this.fix); WriteToOutput(diagnosticsList); } } diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs index b61e90c4a..d768a9ba7 100644 --- a/Engine/ScriptAnalyzer.cs +++ b/Engine/ScriptAnalyzer.cs @@ -1452,12 +1452,12 @@ public Dictionary> CheckRuleExtension(string[] path, PathIn /// /// The path of the file or directory to analyze. /// - /// + /// /// If true, recursively searches the given file path and analyzes any /// script files that are found. /// /// An enumeration of DiagnosticRecords that were found by rules. - public IEnumerable AnalyzePath(string path, bool searchRecursively = false, bool autoFix = false) + public IEnumerable AnalyzePath(string path, bool searchRecursively = false, bool fix = false) { List scriptFilePaths = new List(); @@ -1477,11 +1477,11 @@ public IEnumerable AnalyzePath(string path, bool searchRecursi this.BuildScriptPathList(path, searchRecursively, scriptFilePaths); foreach (string scriptFilePath in scriptFilePaths) { - if (autoFix) + if (fix) { - var scriptFileContentWithAutoFixes = Fix(File.ReadAllText(scriptFilePath)); - // Use UTF8 when writing to avoid issues with special characters such as e.g. the copyright symbol in *.psd1 files. This could be improved to detect the encoding in order to preserve it. - File.WriteAllText(scriptFilePath, scriptFileContentWithAutoFixes, Encoding.UTF8); + var fileEncoding = GetFileEncoding(scriptFilePath); fileEncoding.GetPreamble(); + var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding)); + File.WriteAllText(scriptFilePath, scriptFileContentWithFixes, fileEncoding); // although this preserves the encoding, it will add a BOM to UTF8 files } // Yield each record in the result so that the @@ -1622,6 +1622,18 @@ public EditableText Fix(EditableText text, Range range, out Range updatedRange) return text; } + private static Encoding GetFileEncoding(string path) + { + using (var stream = new FileStream(path, FileMode.Open)) + { + using (var reader = new StreamReader(stream, true)) + { + reader.ReadToEnd(); // needed in order to populate the CurrentEncoding property + return reader.CurrentEncoding; + } + } + } + private static Range SnapToEdges(EditableText text, Range range) { // todo add TextLines.Validate(range) and TextLines.Validate(position) diff --git a/docs/markdown/Invoke-ScriptAnalyzer.md b/docs/markdown/Invoke-ScriptAnalyzer.md index 17ce0526c..f67ecda99 100644 --- a/docs/markdown/Invoke-ScriptAnalyzer.md +++ b/docs/markdown/Invoke-ScriptAnalyzer.md @@ -12,7 +12,7 @@ Evaluates a script or module based on selected best practice rules ### UNNAMED_PARAMETER_SET_1 ``` Invoke-ScriptAnalyzer [-Path] [-CustomRulePath ] [-RecurseCustomRulePath] - [-ExcludeRule ] [-IncludeRule ] [-Severity ] [-Recurse] [-SuppressedOnly] + [-ExcludeRule ] [-IncludeRule ] [-Severity ] [-Recurse] [-SuppressedOnly] [-Fix] [-Settings ] ``` @@ -399,6 +399,22 @@ Accept pipeline input: False Accept wildcard characters: False ``` +### -Fix +Certain warnings contain a suggested fix in their DiagnosticRecord and therefore those warnings will be fixed automatically using this fix. + +When you used Fix, Invoke-ScriptAnalyzer runs as usual but will apply the fixes before running the analysis. Please make sure that you have a backup of your files when using this switch. It tries to pre-server the file encoding but it is possible that a BOM gets added to the fixed files. + +```yaml +Type: SwitchParameter +Parameter Sets: UNNAMED_PARAMETER_SET_1 +Aliases: + +Required: False +Position: Named +Default value: False +Accept pipeline input: False +Accept wildcard characters: False + ### -Settings File path that contains user profile or hash table for ScriptAnalyzer From 24732eee51d0f41dfaf8d1cb16a63fe877dfe44e Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Tue, 12 Sep 2017 23:30:55 +0100 Subject: [PATCH 03/29] PR 817 Minor syntax fix for markdown file --- docs/markdown/Invoke-ScriptAnalyzer.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/markdown/Invoke-ScriptAnalyzer.md b/docs/markdown/Invoke-ScriptAnalyzer.md index f67ecda99..dfec97a6b 100644 --- a/docs/markdown/Invoke-ScriptAnalyzer.md +++ b/docs/markdown/Invoke-ScriptAnalyzer.md @@ -414,6 +414,7 @@ Position: Named Default value: False Accept pipeline input: False Accept wildcard characters: False +``` ### -Settings File path that contains user profile or hash table for ScriptAnalyzer From 4dc4aeb4380f5f34e289ad3cbe02f2eaffb0fca8 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 30 Sep 2017 11:33:45 +0100 Subject: [PATCH 04/29] Improve encoding handling by not using the detectEncodingFromByteOrderMarks overload. Using StreamReader.Peek() instead of StreamReader.ReadToEnd() is sufficient to detect the encoding. This improves the overall behaviour but when run against the PowerShell repo, it will still convert the ASCI psd1 files into UTF8 files. I think this is due to the implementation of the EditableText class. --- Engine/ScriptAnalyzer.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs index d768a9ba7..9dfd41533 100644 --- a/Engine/ScriptAnalyzer.cs +++ b/Engine/ScriptAnalyzer.cs @@ -1479,7 +1479,7 @@ public IEnumerable AnalyzePath(string path, bool searchRecursi { if (fix) { - var fileEncoding = GetFileEncoding(scriptFilePath); fileEncoding.GetPreamble(); + var fileEncoding = GetFileEncoding(scriptFilePath); var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding)); File.WriteAllText(scriptFilePath, scriptFileContentWithFixes, fileEncoding); // although this preserves the encoding, it will add a BOM to UTF8 files } @@ -1626,9 +1626,9 @@ private static Encoding GetFileEncoding(string path) { using (var stream = new FileStream(path, FileMode.Open)) { - using (var reader = new StreamReader(stream, true)) + using (var reader = new StreamReader(stream)) { - reader.ReadToEnd(); // needed in order to populate the CurrentEncoding property + reader.Peek(); // needed in order to populate the CurrentEncoding property return reader.CurrentEncoding; } } From 792fe300ac23391542030468b33d119ec5355d66 Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 6 Nov 2017 19:49:22 +0000 Subject: [PATCH 05/29] improve wording of help as suggested in PR --- docs/markdown/Invoke-ScriptAnalyzer.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/markdown/Invoke-ScriptAnalyzer.md b/docs/markdown/Invoke-ScriptAnalyzer.md index dfec97a6b..b421ea076 100644 --- a/docs/markdown/Invoke-ScriptAnalyzer.md +++ b/docs/markdown/Invoke-ScriptAnalyzer.md @@ -400,9 +400,9 @@ Accept wildcard characters: False ``` ### -Fix -Certain warnings contain a suggested fix in their DiagnosticRecord and therefore those warnings will be fixed automatically using this fix. +Fixes certain warnings which contain a fix in their DiagnosticRecord. -When you used Fix, Invoke-ScriptAnalyzer runs as usual but will apply the fixes before running the analysis. Please make sure that you have a backup of your files when using this switch. It tries to pre-server the file encoding but it is possible that a BOM gets added to the fixed files. +When you used Fix, Invoke-ScriptAnalyzer runs as usual but will apply the fixes before running the analysis. Please make sure that you have a backup of your files when using this switch. It tries to preserve the file encoding but there are still some cases where the encoding can change. ```yaml Type: SwitchParameter From 5a2f47b63b04a8e6bd98238e5599d877282591aa Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 6 Nov 2017 19:51:30 +0000 Subject: [PATCH 06/29] Add simple test for -Fix switch --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 17 +++++++++++++++++ Tests/Engine/TestScriptWithFixableWarnings.ps1 | 5 +++++ 2 files changed, 22 insertions(+) create mode 100644 Tests/Engine/TestScriptWithFixableWarnings.ps1 diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index f1989fae4..cbfb40363 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -465,3 +465,20 @@ Describe "Test CustomizedRulePath" { } } } + +Describe "Test -Fix Switch" { + + It "Fixes warnings" { + # we expect the script to contain warnings + $warningsBeforeFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 + $warningsBeforeFix.Count | Should Be 4 + + # fix the warnings and expect that it should not return the fixed warnings + $warningsWithFixSwitch = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 -Fix + $warningsWithFixSwitch.Count | Should Be 0 + + # double check that the warnings are really fixed + $warningsAfterFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 + $warningsAfterFix.Count | Should Be 0 + } +} diff --git a/Tests/Engine/TestScriptWithFixableWarnings.ps1 b/Tests/Engine/TestScriptWithFixableWarnings.ps1 new file mode 100644 index 000000000..84a7c3076 --- /dev/null +++ b/Tests/Engine/TestScriptWithFixableWarnings.ps1 @@ -0,0 +1,5 @@ +# Produce PSAvoidUsingCmdletAliases warning that should get fixed by replacing it with the actual command +gci . | % { } | ? { } + +# Produces PSAvoidUsingPlainTextForPassword warning that should get fixed by making it a [SecureString] +function Test-bar([string]$PasswordInPlainText){} \ No newline at end of file From 023f9e3249aad4df6516ee921b9b13e9d82f51fb Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 6 Nov 2017 22:51:36 +0000 Subject: [PATCH 07/29] improve test by restoring original file and also asserting against the content. --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 16 ++++++++++++++++ .../TestScriptWithFixableWarnings_AfterFix.ps1 | 5 +++++ 2 files changed, 21 insertions(+) create mode 100644 Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index cbfb40363..61084e738 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -468,6 +468,17 @@ Describe "Test CustomizedRulePath" { Describe "Test -Fix Switch" { + BeforeEach { + $initialTestScript = Get-Content $directory\TestScriptWithFixableWarnings.ps1 -Raw + } + + AfterEach { + if ($null -ne $initialTestScript) + { + $initialTestScript | Set-Content $directory\TestScriptWithFixableWarnings.ps1 -NoNewline + } + } + It "Fixes warnings" { # we expect the script to contain warnings $warningsBeforeFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 @@ -480,5 +491,10 @@ Describe "Test -Fix Switch" { # double check that the warnings are really fixed $warningsAfterFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 $warningsAfterFix.Count | Should Be 0 + + $expectedScriptContentAfterFix = Get-Content $directory\TestScriptWithFixableWarnings_AfterFix.ps1 -Raw + $actualScriptContentAfterFix = Get-Content $directory\TestScriptWithFixableWarnings.ps1 -Raw + write-host $actualScriptContentAfterFix + $actualScriptContentAfterFix | Should Be $expectedScriptContentAfterFix } } diff --git a/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 b/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 new file mode 100644 index 000000000..7898593a2 --- /dev/null +++ b/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 @@ -0,0 +1,5 @@ +# Produce PSAvoidUsingCmdletAliases warning that should get fixed by replacing it with the actual command +Get-ChildItem . | ForEach-Object { } | Where-Object { } + +# Produces PSAvoidUsingPlainTextForPassword warning that should get fixed by making it a [SecureString] +function Test-bar([SecureString]$PasswordInPlainText){} \ No newline at end of file From 6edeef4e7e8bd1ebd88af43ead4a8618e15d6260 Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 6 Nov 2017 23:21:40 +0000 Subject: [PATCH 08/29] Clean up test --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 1 - 1 file changed, 1 deletion(-) diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index 61084e738..b06691276 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -494,7 +494,6 @@ Describe "Test -Fix Switch" { $expectedScriptContentAfterFix = Get-Content $directory\TestScriptWithFixableWarnings_AfterFix.ps1 -Raw $actualScriptContentAfterFix = Get-Content $directory\TestScriptWithFixableWarnings.ps1 -Raw - write-host $actualScriptContentAfterFix $actualScriptContentAfterFix | Should Be $expectedScriptContentAfterFix } } From f129846efe9fffdefcec59e3e9b3e38d7a0f5b70 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 8 Nov 2017 22:11:25 +0000 Subject: [PATCH 09/29] Refactor 'fix' switch out of AnalyzePath method as requested in PR --- .../Commands/InvokeScriptAnalyzerCommand.cs | 9 ++- Engine/ScriptAnalyzer.cs | 70 +++++++++++++------ 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index a393fa1a7..d137abfe0 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -388,7 +388,14 @@ private void ProcessInput() { foreach (var p in processedPaths) { - diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.recurse, this.fix); + if (fix) + { + diagnosticsList = ScriptAnalyzer.Instance.AnalyzeAndFixPath(p, this.recurse); + } + else + { + diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.recurse); + } WriteToOutput(diagnosticsList); } } diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs index 9dfd41533..de32dc3eb 100644 --- a/Engine/ScriptAnalyzer.cs +++ b/Engine/ScriptAnalyzer.cs @@ -1452,40 +1452,44 @@ public Dictionary> CheckRuleExtension(string[] path, PathIn /// /// The path of the file or directory to analyze. /// - /// /// If true, recursively searches the given file path and analyzes any /// script files that are found. /// /// An enumeration of DiagnosticRecords that were found by rules. - public IEnumerable AnalyzePath(string path, bool searchRecursively = false, bool fix = false) + public IEnumerable AnalyzePath(string path, bool searchRecursively = false) { - List scriptFilePaths = new List(); + List scriptFilePaths = ScriptPathList(path, searchRecursively); - if (path == null) + foreach (string scriptFilePath in scriptFilePaths) { - this.outputWriter.ThrowTerminatingError( - new ErrorRecord( - new FileNotFoundException(), - string.Format(CultureInfo.CurrentCulture, Strings.FileNotFound, path), - ErrorCategory.InvalidArgument, - this)); + // Yield each record in the result so that the caller can pull them one at a time + foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) + { + yield return diagnosticRecord; + } } + } + + /// + /// Analyzes a script file or a directory containing script files and fixes warning where possible. + /// + /// The path of the file or directory to analyze. + /// + /// If true, recursively searches the given file path and analyzes any + /// script files that are found. + /// + /// An enumeration of DiagnosticRecords that were found by rules and could not be fixed automatically. + public IEnumerable AnalyzeAndFixPath(string path, bool searchRecursively = false) + { + List scriptFilePaths = ScriptPathList(path, searchRecursively); - // Create in advance the list of script file paths to analyze. This - // is an optimization over doing the whole operation at once - // and calling .Concat on IEnumerables to join results. - this.BuildScriptPathList(path, searchRecursively, scriptFilePaths); foreach (string scriptFilePath in scriptFilePaths) { - if (fix) - { - var fileEncoding = GetFileEncoding(scriptFilePath); - var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding)); - File.WriteAllText(scriptFilePath, scriptFileContentWithFixes, fileEncoding); // although this preserves the encoding, it will add a BOM to UTF8 files - } + var fileEncoding = GetFileEncoding(scriptFilePath); + var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding)); + File.WriteAllText(scriptFilePath, scriptFileContentWithFixes, fileEncoding); - // Yield each record in the result so that the - // caller can pull them one at a time + // Yield each record in the result so that the caller can pull them one at a time foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) { yield return diagnosticRecord; @@ -1676,6 +1680,28 @@ private static EditableText Fix( return text; } + private List ScriptPathList(string path, bool searchRecursively) + { + List scriptFilePaths = new List(); + + if (path == null) + { + this.outputWriter.ThrowTerminatingError( + new ErrorRecord( + new FileNotFoundException(), + string.Format(CultureInfo.CurrentCulture, Strings.FileNotFound, path), + ErrorCategory.InvalidArgument, + this)); + } + + // Create in advance the list of script file paths to analyze. This + // is an optimization over doing the whole operation at once + // and calling .Concat on IEnumerables to join results. + this.BuildScriptPathList(path, searchRecursively, scriptFilePaths); + + return scriptFilePaths; + } + private void BuildScriptPathList( string path, bool searchRecursively, From bfa1c5457ce2a96aaca44d4cde2593aee48293e8 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 8 Nov 2017 22:46:24 +0000 Subject: [PATCH 10/29] Implement SupportShouldProcess for InvokeScriptAnalyzerCommand down to a per process path and per file basis. Propagating it further down would not make sense because then it would need to analyze all files, in this case SupportShouldContinue could be implemented in the future. --- .../Commands/InvokeScriptAnalyzerCommand.cs | 7 ++-- Engine/ScriptAnalyzer.cs | 34 ++++++++++++------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index d137abfe0..40b8d5cff 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -38,6 +38,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands [Cmdlet(VerbsLifecycle.Invoke, "ScriptAnalyzer", DefaultParameterSetName = "File", + SupportsShouldProcess = true, HelpUri = "http://go.microsoft.com/fwlink/?LinkId=525914")] public class InvokeScriptAnalyzerCommand : PSCmdlet, IOutputWriter { @@ -390,11 +391,13 @@ private void ProcessInput() { if (fix) { - diagnosticsList = ScriptAnalyzer.Instance.AnalyzeAndFixPath(p, this.recurse); + ShouldProcess(p, $"Analyzing and fixing path with Recurse={this.recurse}"); + diagnosticsList = ScriptAnalyzer.Instance.AnalyzeAndFixPath(p, this.ShouldProcess, this.recurse); } else { - diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.recurse); + ShouldProcess(p, $"Analyzing path with Recurse={this.recurse}"); + diagnosticsList = ScriptAnalyzer.Instance.AnalyzePath(p, this.ShouldProcess, this.recurse); } WriteToOutput(diagnosticsList); } diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs index de32dc3eb..ce946f029 100644 --- a/Engine/ScriptAnalyzer.cs +++ b/Engine/ScriptAnalyzer.cs @@ -1444,28 +1444,32 @@ public Dictionary> CheckRuleExtension(string[] path, PathIn return results; } -#endregion + #endregion /// /// Analyzes a script file or a directory containing script files. /// /// The path of the file or directory to analyze. + /// Whether the action should be executed. /// /// If true, recursively searches the given file path and analyzes any /// script files that are found. /// /// An enumeration of DiagnosticRecords that were found by rules. - public IEnumerable AnalyzePath(string path, bool searchRecursively = false) + public IEnumerable AnalyzePath(string path, Func shouldProcess, bool searchRecursively = false) { List scriptFilePaths = ScriptPathList(path, searchRecursively); foreach (string scriptFilePath in scriptFilePaths) { - // Yield each record in the result so that the caller can pull them one at a time - foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) + if (shouldProcess(scriptFilePath, $"Analyzing file {scriptFilePath}")) { - yield return diagnosticRecord; + // Yield each record in the result so that the caller can pull them one at a time + foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) + { + yield return diagnosticRecord; + } } } } @@ -1474,25 +1478,29 @@ public IEnumerable AnalyzePath(string path, bool searchRecursi /// Analyzes a script file or a directory containing script files and fixes warning where possible. /// /// The path of the file or directory to analyze. + /// Whether the action should be executed. /// /// If true, recursively searches the given file path and analyzes any /// script files that are found. /// /// An enumeration of DiagnosticRecords that were found by rules and could not be fixed automatically. - public IEnumerable AnalyzeAndFixPath(string path, bool searchRecursively = false) + public IEnumerable AnalyzeAndFixPath(string path, Func shouldProcess, bool searchRecursively = false) { List scriptFilePaths = ScriptPathList(path, searchRecursively); foreach (string scriptFilePath in scriptFilePaths) { - var fileEncoding = GetFileEncoding(scriptFilePath); - var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding)); - File.WriteAllText(scriptFilePath, scriptFileContentWithFixes, fileEncoding); - - // Yield each record in the result so that the caller can pull them one at a time - foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) + if (shouldProcess(scriptFilePath, $"Analyzing and fixing file {scriptFilePath}")) { - yield return diagnosticRecord; + var fileEncoding = GetFileEncoding(scriptFilePath); + var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding)); + File.WriteAllText(scriptFilePath, scriptFileContentWithFixes, fileEncoding); + + // Yield each record in the result so that the caller can pull them one at a time + foreach (var diagnosticRecord in this.AnalyzeFile(scriptFilePath)) + { + yield return diagnosticRecord; + } } } } From 50af5d20a9dd86640e0aabf5e625ed1e24c4fde0 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 8 Nov 2017 23:07:14 +0000 Subject: [PATCH 11/29] Adapt the full .net wrapper in LibraryUsage.tests.ps1 with the -fix switch and the new SupportsShouldProcess Func --- Tests/Engine/LibraryUsage.tests.ps1 | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Tests/Engine/LibraryUsage.tests.ps1 b/Tests/Engine/LibraryUsage.tests.ps1 index 64570d58f..89c41e416 100644 --- a/Tests/Engine/LibraryUsage.tests.ps1 +++ b/Tests/Engine/LibraryUsage.tests.ps1 @@ -48,7 +48,10 @@ function Invoke-ScriptAnalyzer { [switch] $IncludeDefaultRules, [Parameter(Mandatory = $false)] - [switch] $SuppressedOnly + [switch] $SuppressedOnly, + + [Parameter(Mandatory = $false)] + [switch] $Fix ) if ($null -eq $CustomRulePath) @@ -75,7 +78,8 @@ function Invoke-ScriptAnalyzer { ); if ($PSCmdlet.ParameterSetName -eq "File") { - return $scriptAnalyzer.AnalyzePath($Path, $Recurse.IsPresent); + $supportsShouldProcessFunc = [Func[bool, string,string]]{ return $Recurse.IsPresent } + $scriptAnalyzer.AnalyzePath($Path, $supportsShouldProcessFunc, $Recurse.IsPresent); } else { return $scriptAnalyzer.AnalyzeScriptDefinition($ScriptDefinition); From 771ffb6176594cbe5f2996ce2efa33e4f994dab8 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 8 Nov 2017 23:24:00 +0000 Subject: [PATCH 12/29] Fix Func wrapper in LibraryUsage.tests.ps1 --- Tests/Engine/LibraryUsage.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/LibraryUsage.tests.ps1 b/Tests/Engine/LibraryUsage.tests.ps1 index 89c41e416..e28035abc 100644 --- a/Tests/Engine/LibraryUsage.tests.ps1 +++ b/Tests/Engine/LibraryUsage.tests.ps1 @@ -78,7 +78,7 @@ function Invoke-ScriptAnalyzer { ); if ($PSCmdlet.ParameterSetName -eq "File") { - $supportsShouldProcessFunc = [Func[bool, string,string]]{ return $Recurse.IsPresent } + $supportsShouldProcessFunc = [Func[string, string, bool]]{ return $Recurse.IsPresent } $scriptAnalyzer.AnalyzePath($Path, $supportsShouldProcessFunc, $Recurse.IsPresent); } else { From 701c2640c840d0aec7fbf217f6c99cc4b363ad55 Mon Sep 17 00:00:00 2001 From: Chris Date: Wed, 8 Nov 2017 23:34:03 +0000 Subject: [PATCH 13/29] Another fix to LibraryUsage.tests.ps1 to accomodate -Fix switch and also return --- Tests/Engine/LibraryUsage.tests.ps1 | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Tests/Engine/LibraryUsage.tests.ps1 b/Tests/Engine/LibraryUsage.tests.ps1 index e28035abc..440ce8889 100644 --- a/Tests/Engine/LibraryUsage.tests.ps1 +++ b/Tests/Engine/LibraryUsage.tests.ps1 @@ -78,8 +78,12 @@ function Invoke-ScriptAnalyzer { ); if ($PSCmdlet.ParameterSetName -eq "File") { - $supportsShouldProcessFunc = [Func[string, string, bool]]{ return $Recurse.IsPresent } - $scriptAnalyzer.AnalyzePath($Path, $supportsShouldProcessFunc, $Recurse.IsPresent); + $supportsShouldProcessFunc = [Func[string, string, bool]]{ return $Recurse.IsPresent } + if ($Fix.IsPresent) + { + return $scriptAnalyzer.AnalyzeAndFixPath($Path, $supportsShouldProcessFunc, $Recurse.IsPresent); + } + return $scriptAnalyzer.AnalyzePath($Path, $supportsShouldProcessFunc, $Recurse.IsPresent); } else { return $scriptAnalyzer.AnalyzeScriptDefinition($ScriptDefinition); From a16c232f6b4c8dc168bd6e0caf757a2a5fc0183f Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 05:56:11 +0000 Subject: [PATCH 14/29] Always return true for shouldprocess --- Tests/Engine/LibraryUsage.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/LibraryUsage.tests.ps1 b/Tests/Engine/LibraryUsage.tests.ps1 index 440ce8889..30c6579da 100644 --- a/Tests/Engine/LibraryUsage.tests.ps1 +++ b/Tests/Engine/LibraryUsage.tests.ps1 @@ -78,7 +78,7 @@ function Invoke-ScriptAnalyzer { ); if ($PSCmdlet.ParameterSetName -eq "File") { - $supportsShouldProcessFunc = [Func[string, string, bool]]{ return $Recurse.IsPresent } + $supportsShouldProcessFunc = [Func[string, string, bool]]{ return $true } if ($Fix.IsPresent) { return $scriptAnalyzer.AnalyzeAndFixPath($Path, $supportsShouldProcessFunc, $Recurse.IsPresent); From e64335f0369edfe7664b741d57cccaaa99042ba1 Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 06:18:27 +0000 Subject: [PATCH 15/29] Add supportshouldprocess to full .net wrapper library --- Tests/Engine/LibraryUsage.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/LibraryUsage.tests.ps1 b/Tests/Engine/LibraryUsage.tests.ps1 index 30c6579da..6d81a39ca 100644 --- a/Tests/Engine/LibraryUsage.tests.ps1 +++ b/Tests/Engine/LibraryUsage.tests.ps1 @@ -15,7 +15,7 @@ $directory = Split-Path -Parent $MyInvocation.MyCommand.Path # wraps the usage of ScriptAnalyzer as a .NET library function Invoke-ScriptAnalyzer { param ( - [CmdletBinding(DefaultParameterSetName="File")] + [CmdletBinding(DefaultParameterSetName="File", SupportsShouldProcess = $true)] [parameter(Mandatory = $true, Position = 0, ParameterSetName="File")] [Alias("PSPath")] From 4297ff4e3c3d7a7f98c7aeea75948dfc5eee8abd Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 09:45:37 +0100 Subject: [PATCH 16/29] Add 'WhatIf' to common words for help tests --- Tests/Engine/ModuleHelp.Tests.ps1 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/Engine/ModuleHelp.Tests.ps1 b/Tests/Engine/ModuleHelp.Tests.ps1 index f1b9e5743..aeb84892a 100644 --- a/Tests/Engine/ModuleHelp.Tests.ps1 +++ b/Tests/Engine/ModuleHelp.Tests.ps1 @@ -121,7 +121,7 @@ function Get-ParametersDefaultFirst { ) BEGIN { - $Common = 'Debug', 'ErrorAction', 'ErrorVariable', 'InformationAction', 'InformationVariable', 'OutBuffer', 'OutVariable', 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable' + $Common = 'Debug', 'ErrorAction', 'ErrorVariable', 'InformationAction', 'InformationVariable', 'OutBuffer', 'OutVariable', 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable', 'WhatIf' $parameters = @() } PROCESS { @@ -266,7 +266,7 @@ foreach ($command in $commands) { Context "Test parameter help for $commandName" { $Common = 'Debug', 'ErrorAction', 'ErrorVariable', 'InformationAction', 'InformationVariable', 'OutBuffer', 'OutVariable', - 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable' + 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable', 'WhatIf' # Get parameters. When >1 parameter with same name, # get parameter from the default parameter set, if any. @@ -315,4 +315,4 @@ foreach ($command in $commands) { } } } -} \ No newline at end of file +} From 9785993b96703287791eebff54f3d73011577eca Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 09:57:13 +0100 Subject: [PATCH 17/29] Add 'Confirm' to common words for help tests --- Tests/Engine/ModuleHelp.Tests.ps1 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Engine/ModuleHelp.Tests.ps1 b/Tests/Engine/ModuleHelp.Tests.ps1 index aeb84892a..b4dd7e94a 100644 --- a/Tests/Engine/ModuleHelp.Tests.ps1 +++ b/Tests/Engine/ModuleHelp.Tests.ps1 @@ -121,7 +121,7 @@ function Get-ParametersDefaultFirst { ) BEGIN { - $Common = 'Debug', 'ErrorAction', 'ErrorVariable', 'InformationAction', 'InformationVariable', 'OutBuffer', 'OutVariable', 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable', 'WhatIf' + $Common = 'Debug', 'ErrorAction', 'ErrorVariable', 'InformationAction', 'InformationVariable', 'OutBuffer', 'OutVariable', 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable', 'WhatIf', 'Confirm' $parameters = @() } PROCESS { @@ -266,7 +266,7 @@ foreach ($command in $commands) { Context "Test parameter help for $commandName" { $Common = 'Debug', 'ErrorAction', 'ErrorVariable', 'InformationAction', 'InformationVariable', 'OutBuffer', 'OutVariable', - 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable', 'WhatIf' + 'PipelineVariable', 'Verbose', 'WarningAction', 'WarningVariable', 'WhatIf', 'Confirm' # Get parameters. When >1 parameter with same name, # get parameter from the default parameter set, if any. From 14197ce962cdf436da42de26296acebbf827bf96 Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 10:01:04 +0100 Subject: [PATCH 18/29] Add debug line to debug failing test --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index b06691276..56dd0c990 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -482,6 +482,7 @@ Describe "Test -Fix Switch" { It "Fixes warnings" { # we expect the script to contain warnings $warningsBeforeFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 + $warningsBeforeFix | % { Write-Verbose $_ -Verbose } $warningsBeforeFix.Count | Should Be 4 # fix the warnings and expect that it should not return the fixed warnings From fffcd7615fb3ec22a32413055be50c967a47b706 Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 10:10:36 +0100 Subject: [PATCH 19/29] Add more debug info for test failure --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index 56dd0c990..f4c065d1f 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -482,7 +482,7 @@ Describe "Test -Fix Switch" { It "Fixes warnings" { # we expect the script to contain warnings $warningsBeforeFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 - $warningsBeforeFix | % { Write-Verbose $_ -Verbose } + $warningsBeforeFix | % { Write-Verbose "$($_.Message)" -Verbose } $warningsBeforeFix.Count | Should Be 4 # fix the warnings and expect that it should not return the fixed warnings From c707a1e392a47b4ac9f0403ea4aab91c090f8e28 Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 10:22:03 +0100 Subject: [PATCH 20/29] Add comment about deliberate whitespace warning --- Tests/Engine/TestScriptWithFixableWarnings.ps1 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Engine/TestScriptWithFixableWarnings.ps1 b/Tests/Engine/TestScriptWithFixableWarnings.ps1 index 84a7c3076..f191b3c70 100644 --- a/Tests/Engine/TestScriptWithFixableWarnings.ps1 +++ b/Tests/Engine/TestScriptWithFixableWarnings.ps1 @@ -1,5 +1,5 @@ -# Produce PSAvoidUsingCmdletAliases warning that should get fixed by replacing it with the actual command +# Produce PSAvoidUsingCmdletAliases and PSAvoidTrailingWhitespace warnings that should get fixed by replacing it with the actual command gci . | % { } | ? { } # Produces PSAvoidUsingPlainTextForPassword warning that should get fixed by making it a [SecureString] -function Test-bar([string]$PasswordInPlainText){} \ No newline at end of file +function Test-bar([string]$PasswordInPlainText){} From 9d4e8efee8af83466645e727f1664b49950a1191 Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 10:23:37 +0100 Subject: [PATCH 21/29] Number of expected warnings now 5 in -fix test due to PSAvoidTrailingWhitespace --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index f4c065d1f..5a0adb0a2 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -483,7 +483,7 @@ Describe "Test -Fix Switch" { # we expect the script to contain warnings $warningsBeforeFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 $warningsBeforeFix | % { Write-Verbose "$($_.Message)" -Verbose } - $warningsBeforeFix.Count | Should Be 4 + $warningsBeforeFix.Count | Should Be 5 # fix the warnings and expect that it should not return the fixed warnings $warningsWithFixSwitch = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 -Fix From c0f881d5b8ec5eec5ef68dc67efad12f8f37f14a Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 10:43:03 +0100 Subject: [PATCH 22/29] Update expected test file --- Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 b/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 index 7898593a2..5b5db011e 100644 --- a/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 +++ b/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 @@ -1,5 +1,5 @@ -# Produce PSAvoidUsingCmdletAliases warning that should get fixed by replacing it with the actual command +# Produce PSAvoidUsingCmdletAliases and PSAvoidTrailingWhitespace warnings that should get fixed by replacing it with the actual command Get-ChildItem . | ForEach-Object { } | Where-Object { } # Produces PSAvoidUsingPlainTextForPassword warning that should get fixed by making it a [SecureString] -function Test-bar([SecureString]$PasswordInPlainText){} \ No newline at end of file +function Test-bar([SecureString]$PasswordInPlainText){} From c59b6b0b83b3b7e6b43fd77fba028ec960a6592c Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 10:51:01 +0100 Subject: [PATCH 23/29] Remove trailing whitespace in expected file since this gets fixed now --- Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 b/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 index 5b5db011e..876e8ed8e 100644 --- a/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 +++ b/Tests/Engine/TestScriptWithFixableWarnings_AfterFix.ps1 @@ -1,5 +1,5 @@ # Produce PSAvoidUsingCmdletAliases and PSAvoidTrailingWhitespace warnings that should get fixed by replacing it with the actual command -Get-ChildItem . | ForEach-Object { } | Where-Object { } +Get-ChildItem . | ForEach-Object { } | Where-Object { } # Produces PSAvoidUsingPlainTextForPassword warning that should get fixed by making it a [SecureString] function Test-bar([SecureString]$PasswordInPlainText){} From 6f6896a13b12c6418542b33113306a2d4f167841 Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 10:58:18 +0100 Subject: [PATCH 24/29] Remove debugging line since test passes now --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 1 - 1 file changed, 1 deletion(-) diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index 5a0adb0a2..a4fcf3f59 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -482,7 +482,6 @@ Describe "Test -Fix Switch" { It "Fixes warnings" { # we expect the script to contain warnings $warningsBeforeFix = Invoke-ScriptAnalyzer $directory\TestScriptWithFixableWarnings.ps1 - $warningsBeforeFix | % { Write-Verbose "$($_.Message)" -Verbose } $warningsBeforeFix.Count | Should Be 5 # fix the warnings and expect that it should not return the fixed warnings From f5ac9eb16db151c926a30a8fe590b24d64f24741 Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 11:12:07 +0100 Subject: [PATCH 25/29] Set-Content -NoNewline was only introduced in PS v5 and would therefore not work -> replace with .net method --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index a4fcf3f59..27ede4dc3 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -475,7 +475,7 @@ Describe "Test -Fix Switch" { AfterEach { if ($null -ne $initialTestScript) { - $initialTestScript | Set-Content $directory\TestScriptWithFixableWarnings.ps1 -NoNewline + [System.IO.File]::WriteAllText($directory\TestScriptWithFixableWarnings.ps1, initialTestScript) # Set-Content -NoNewline was only introduced in PS v5 and would therefore not work in CI } } From 8e4675019089317d1d0bf6712b143e4d5c7f5cb1 Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 11:27:52 +0100 Subject: [PATCH 26/29] Syntax fix of last commit --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index 27ede4dc3..5b2f2749d 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -475,7 +475,7 @@ Describe "Test -Fix Switch" { AfterEach { if ($null -ne $initialTestScript) { - [System.IO.File]::WriteAllText($directory\TestScriptWithFixableWarnings.ps1, initialTestScript) # Set-Content -NoNewline was only introduced in PS v5 and would therefore not work in CI + [System.IO.File]::WriteAllText("$($directory)\TestScriptWithFixableWarnings.ps1", initialTestScript) # Set-Content -NoNewline was only introduced in PS v5 and would therefore not work in CI } } From e184e3b9b3fef3c1884bd81773b73686e739ddee Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 11:43:47 +0100 Subject: [PATCH 27/29] Another small syntax fix (sorry, I am commiting from my mobile) --- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index 5b2f2749d..a915bed85 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -475,7 +475,7 @@ Describe "Test -Fix Switch" { AfterEach { if ($null -ne $initialTestScript) { - [System.IO.File]::WriteAllText("$($directory)\TestScriptWithFixableWarnings.ps1", initialTestScript) # Set-Content -NoNewline was only introduced in PS v5 and would therefore not work in CI + [System.IO.File]::WriteAllText("$($directory)\TestScriptWithFixableWarnings.ps1", $initialTestScript) # Set-Content -NoNewline was only introduced in PS v5 and would therefore not work in CI } } From 0df445b26659372cfaeac4d9fda9d399064fffa1 Mon Sep 17 00:00:00 2001 From: bergmeister Date: Thu, 9 Nov 2017 12:42:20 +0100 Subject: [PATCH 28/29] Return $pscmdlet.shouldprocess in .net test wrapper --- Tests/Engine/LibraryUsage.tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Engine/LibraryUsage.tests.ps1 b/Tests/Engine/LibraryUsage.tests.ps1 index 6d81a39ca..a945fdd02 100644 --- a/Tests/Engine/LibraryUsage.tests.ps1 +++ b/Tests/Engine/LibraryUsage.tests.ps1 @@ -78,7 +78,7 @@ function Invoke-ScriptAnalyzer { ); if ($PSCmdlet.ParameterSetName -eq "File") { - $supportsShouldProcessFunc = [Func[string, string, bool]]{ return $true } + $supportsShouldProcessFunc = [Func[string, string, bool]]{ return $PSCmdlet.Shouldprocess } if ($Fix.IsPresent) { return $scriptAnalyzer.AnalyzeAndFixPath($Path, $supportsShouldProcessFunc, $Recurse.IsPresent); From 8525e037316d898eb2d6587bc4e61ff562b283fc Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 13 Nov 2017 22:48:22 +0000 Subject: [PATCH 29/29] Optimize -Fix switch to only write the fixed file to disk if any fixes were actually applied. This helps especially because sometimes the encoding cannot be preserved (which is probably due to the EditableText implementation) --- Engine/Formatter.cs | 3 ++- Engine/ScriptAnalyzer.cs | 20 ++++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/Engine/Formatter.cs b/Engine/Formatter.cs index 098e772a9..bcdbc32b8 100644 --- a/Engine/Formatter.cs +++ b/Engine/Formatter.cs @@ -53,7 +53,8 @@ public static string Format( ScriptAnalyzer.Instance.Initialize(cmdlet, null, null, null, null, true, false); Range updatedRange; - text = ScriptAnalyzer.Instance.Fix(text, range, out updatedRange); + bool fixesWereApplied; + text = ScriptAnalyzer.Instance.Fix(text, range, out updatedRange, out fixesWereApplied); range = updatedRange; } diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs index ce946f029..cdc3884f5 100644 --- a/Engine/ScriptAnalyzer.cs +++ b/Engine/ScriptAnalyzer.cs @@ -1493,8 +1493,12 @@ public IEnumerable AnalyzeAndFixPath(string path, Func AnalyzeScriptDefinition(string scriptDefini /// Fix the violations in the given script text. /// /// The script text to be fixed. + /// Whether any warnings were fixed. /// The fixed script text. - public string Fix(string scriptDefinition) + public string Fix(string scriptDefinition, out bool fixesWereApplied) { if (scriptDefinition == null) { @@ -1561,7 +1566,7 @@ public string Fix(string scriptDefinition) } Range updatedRange; - return Fix(new EditableText(scriptDefinition), null, out updatedRange).ToString(); + return Fix(new EditableText(scriptDefinition), null, out updatedRange, out fixesWereApplied).ToString(); } /// @@ -1570,8 +1575,9 @@ public string Fix(string scriptDefinition) /// An object of type `EditableText` that encapsulates the script text to be fixed. /// The range in which the fixes are allowed. /// The updated range after the fixes have been applied. + /// Whether any warnings were fixed. /// The same instance of `EditableText` that was passed to the method, but the instance encapsulates the fixed script text. This helps in chaining the Fix method. - public EditableText Fix(EditableText text, Range range, out Range updatedRange) + public EditableText Fix(EditableText text, Range range, out Range updatedRange, out bool fixesWereApplied) { if (text == null) { @@ -1604,7 +1610,9 @@ public EditableText Fix(EditableText text, Range range, out Range updatedRange) this.outputWriter.WriteVerbose($"Found {corrections.Count} violations."); int unusedCorrections; Fix(text, corrections, out unusedCorrections); - this.outputWriter.WriteVerbose($"Fixed {corrections.Count - unusedCorrections} violations."); + var numberOfFixedViolatons = corrections.Count - unusedCorrections; + fixesWereApplied = numberOfFixedViolatons > 0; + this.outputWriter.WriteVerbose($"Fixed {numberOfFixedViolatons} violations."); // This is an indication of an infinite loop. There is a small chance of this. // It is better to abort the fixing operation at this point.