Add Fix switch parameter for 'File' parameter set to auto-correct warnings (#802) #817

bergmeister · 2017-08-30T21:58:46Z

Implements #802 (auto-correct warnings) by adding an AutoFix switch parameter for 'File' parameter set, which uses the SuggestedCorrectionExtent information as a correction.

It uses UTF8 to overwrite the existing file with the fix to avoid problems with special characters such as the copyright symbol in psd1 files but this could be enhanced to preserve the encoding of the original file in the future.
This is a a minimum (and hopefully) viable implementation of issue #802. It works when being run against e.g. the whole PowerShell repo here but throws an 'EditableTextInvalidLineEnding' error at some point for one file (this seems to be rather an issue with existing code that fixes EditableText)

… 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)

bergmeister · 2017-08-30T22:54:45Z

@kapilmb Only 3 tests that were asserting against the help of the new switch parameter failed, could you give me a pointer please where I could fix them as it did not seem to be obvious to me.

kapilmb · 2017-09-08T20:34:20Z

Thanks for PR.

To resolve the test failure you will need to add the parameter related help to docs/markdown/Invoke-ScriptAnalyzer.md.

I remember you suggesting adding ShouldProcess. I think we should add it before this makes it to the development branch.

kapilmb · 2017-09-08T20:31:12Z

Engine/Commands/InvokeScriptAnalyzerCommand.cs

+        /// Resolves rule violations automatically where possible.
+        /// </summary>
+        [Parameter(Mandatory = false, ParameterSetName = "File")]
+        public SwitchParameter AutoFix


Fix vs AutoFix ? I think Fix. It conveys the some information and is shorter.

Ok. I Agree with Fix

kapilmb · 2017-09-08T20:31:21Z

Engine/ScriptAnalyzer.cs

+                {
+                    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);


When we modify a file, we should preserve the file encoding and line endings

Agreed. A quick search showed however that detecting the encoding is non-trivial and difficult to get 100% correct. Should we do something like this?

The StreamReader object provides encoding of a file. I think we should that instead of rewriting the logic to detect the encoding.

kapilmb · 2017-09-08T20:33:31Z

Engine/ScriptAnalyzer.cs

@@ -1475,6 +1477,13 @@ public IEnumerable<DiagnosticRecord> AnalyzePath(string path, bool searchRecursi
            this.BuildScriptPathList(path, searchRecursively, scriptFilePaths);
            foreach (string scriptFilePath in scriptFilePaths)
            {
+                if (autoFix)
+                {


I think the right place for this block is in the InvokeScriptAnalyzerCommand.cs file. We do not need to change behavior 'Analyzepath` method, which should only analyze and not fix.

This would require some re-factoring of the project because one needs to call the private method BuildScriptPathList of the ScriptAnalyzer class first to get the list of files, therefore it would probably be best to extract the BuildScriptPathList method into a separate class. Can you confirm that you really want this or suggest other options?

… in the case of UTF8, a BOM will get added. Update help markdown to fix help related tests.

bergmeister · 2017-09-12T22:43:31Z

@kapilmb I renamed the switch to Fix and edited the markdown help file to fix the tests. I improved the code to detect and use the encoding. When test running it against the PowerShell repo, I found that it generally works fine but it converts some the psd1 files in ASCII to UTF8, which is probably due to the implementation of the EditableText class. However I think the current implementation should be sufficient due to delivered value.

…rMarks 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.

bergmeister · 2017-10-16T19:41:26Z

@kapilmb What do you think about the current implementation?

bergmeister · 2017-11-03T19:54:58Z

@SteveL-MSFT : Is @kapilmb still maintaining this repo because it seems to have become stale?

SteveL-MSFT · 2017-11-03T21:46:13Z

Yes, @kapilmb is still maintaining for now, but due to some resource shuffling internally, he may not be as available. @adityapatwardhan @JamesWTruher can you guys help out on this one?

adityapatwardhan · 2017-11-06T17:27:11Z

Engine/ScriptAnalyzer.cs

@@ -1475,6 +1477,13 @@ public IEnumerable<DiagnosticRecord> AnalyzePath(string path, bool searchRecursi
            this.BuildScriptPathList(path, searchRecursively, scriptFilePaths);
            foreach (string scriptFilePath in scriptFilePaths)
            {
+                if (fix)


Can some tests be added to test new code?

adityapatwardhan · 2017-11-06T17:30:25Z

docs/markdown/Invoke-ScriptAnalyzer.md

@@ -399,6 +399,23 @@ 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.


I suggest a bit of rewording to be like other switch parameters

Fixes certain warnings which contain a fix in their DiagnosticRecord.

adityapatwardhan · 2017-11-06T17:31:13Z

docs/markdown/Invoke-ScriptAnalyzer.md

+### -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.


pre-server -> preserve.

bergmeister · 2017-11-06T20:24:15Z

I applied your suggested changes but NuGet has a global outage at the moment (see here). Can you re-queue a build later again please?

SteveL-MSFT · 2017-11-06T22:43:15Z

@bergmeister Nuget.org is back, I restarted your PR build

…e content.

… into AutoFixWarnings

…nalyzer into AutoFixWarnings

bergmeister · 2017-11-06T23:25:50Z

Thanks Steve.
I improved the test in the meantime and downstreamed the latest changes. From what I can see in the build log is that the tests are run twice and the 2nd time the -Fix switch parameter is not present and therefore fails. I do not know all the details of the build but my local build for PowerShell Core and Windows PowerShell is OK and running the test file itself is fine but once I run Invoke-Pester I get similar failures where suddenly the -Fix is missing and it seems to me that this must be due to some other test before. I would appreciate help from someone that is more familiar with the whole build process because even the uploaded artifacts does not seem to have the -Fix switch on it (but I can see it in the help)

kapilmb · 2017-11-08T02:02:57Z

@bergmeister Sorry I haven't been able to give any time here. I saw the test failures and it looks like you need to modify Tests/Engine/LibraryUsage.tests.ps1 for the tests to pass. I would recommend the following two things before considering this PR to be complete:

Please remove the fix parameter from the AnalyzePath and provide an alternate implementation of your logic. The method, AnalyzePath should only analyze and not be in the business of fixing. This would be in line with the coding best practice to have one function do only one thing.
We need to support ShouldProcess when using -Fix

…o 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.

…witch and the new SupportsShouldProcess Func

…lso return

…Whitespace

…re not work -> replace with .net method

bergmeister · 2017-11-09T11:02:53Z

@kapilmb Thanks for your pointers and review. I have implemented your suggestions and fixed the tests. Due to the many commits that I made, I think it would make sense to squash the merge btw.

…s were actually applied. This helps especially because sometimes the encoding cannot be preserved (which is probably due to the EditableText implementation)

…riptAnalyzer into AutoFixWarnings

kapilmb · 2017-11-15T08:08:22Z

@bergmeister Thanks for the changes. From a cursory glance things look complete, but I will look at it more carefully in the next 1-2 days and get back to you. We should be able to get this thing in soon. Thanks for your patience.

bergmeister · 2017-12-02T18:32:42Z

Thanks. :-) Do you know when the next Release is planned?

JamesLear92 · 2018-04-06T15:12:41Z

So this was merged? I can see references to the -Fix Param in https://github.com/PowerShell/PSScriptAnalyzer readme, but I can't see the param in the module I downloaded today.

bergmeister · 2018-04-06T15:44:03Z

@Omniusz It was merged into the development branch but is not released yet. The default view of the Github page shows you the development branch. For the documentation about the latest release, please refer to the master branch here: https://github.com/PowerShell/PSScriptAnalyzer/tree/master
P.S. We are expecting to release soon but I cannot say a date yet. If you want to I could give you a private preview build if you already want to use this new feature (the artifacts from the AppVeyor builds are not suitable for that unfortunately for unknown reasons)

msftclas added the cla-already-signed label Aug 30, 2017

kapilmb suggested changes Sep 8, 2017

View reviewed changes

bergmeister added 2 commits September 12, 2017 23:22

PR 817: Name switch 'Fix' and preserver Encoding of the file. However…

ef75719

… in the case of UTF8, a BOM will get added. Update help markdown to fix help related tests.

PR 817 Minor syntax fix for markdown file

24732ee

bergmeister changed the title ~~Add AutoFix switch parameter for 'File' parameter set to auto-correct warnings (#802)~~ Add Fix switch parameter for 'File' parameter set to auto-correct warnings (#802) Sep 13, 2017

msftgits removed the cla-already-signed label Sep 26, 2017

PowerShell deleted a comment from msftclas Sep 27, 2017

SteveL-MSFT requested review from adityapatwardhan and JamesWTruher November 3, 2017 21:45

adityapatwardhan suggested changes Nov 6, 2017

View reviewed changes

bergmeister added 2 commits November 6, 2017 19:49

improve wording of help as suggested in PR

792fe30

Add simple test for -Fix switch

5a2f47b

bergmeister added 4 commits November 6, 2017 22:51

improve test by restoring original file and also asserting against th…

023f9e3

…e content.

Merge branch 'master' of https://github.com/PowerShell/PSScriptAnalyzer…

5d0b52d

… into AutoFixWarnings

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

78d9663

…nalyzer into AutoFixWarnings

Clean up test

6edeef4

bergmeister added 2 commits November 8, 2017 22:11

Refactor 'fix' switch out of AnalyzePath method as requested in PR

f129846

Implement SupportShouldProcess for InvokeScriptAnalyzerCommand down t…

bfa1c54

…o 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.

bergmeister added 17 commits November 8, 2017 23:07

Adapt the full .net wrapper in LibraryUsage.tests.ps1 with the -fix s…

50af5d2

…witch and the new SupportsShouldProcess Func

Fix Func wrapper in LibraryUsage.tests.ps1

771ffb6

Another fix to LibraryUsage.tests.ps1 to accomodate -Fix switch and a…

701c264

…lso return

Always return true for shouldprocess

a16c232

Add supportshouldprocess to full .net wrapper library

e64335f

Add 'WhatIf' to common words for help tests

4297ff4

Add 'Confirm' to common words for help tests

9785993

Add debug line to debug failing test

14197ce

Add more debug info for test failure

fffcd76

Add comment about deliberate whitespace warning

c707a1e

Number of expected warnings now 5 in -fix test due to PSAvoidTrailing…

9d4e8ef

…Whitespace

Update expected test file

c0f881d

Remove trailing whitespace in expected file since this gets fixed now

c59b6b0

Remove debugging line since test passes now

6f6896a

Set-Content -NoNewline was only introduced in PS v5 and would therefo…

f5ac9eb

…re not work -> replace with .net method

Syntax fix of last commit

8e46750

Another small syntax fix (sorry, I am commiting from my mobile)

e184e3b

bergmeister added 3 commits November 9, 2017 12:42

Return $pscmdlet.shouldprocess in .net test wrapper

0df445b

Optimize -Fix switch to only write the fixed file to disk if any fixe…

8525e03

…s were actually applied. This helps especially because sometimes the encoding cannot be preserved (which is probably due to the EditableText implementation)

Merge branch 'AutoFixWarnings' of https://github.com/bergmeister/PSSc…

e12c4a6

…riptAnalyzer into AutoFixWarnings

kapilmb approved these changes Nov 30, 2017

View reviewed changes

kapilmb merged commit 2056fe0 into PowerShell:development Nov 30, 2017

bergmeister mentioned this pull request Jan 20, 2018

Add documentation for -Fix switch to README.md #852

Merged

bergmeister mentioned this pull request Feb 4, 2018

Added -ReportSummary switch #868

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Fix switch parameter for 'File' parameter set to auto-correct warnings (#802) #817

Add Fix switch parameter for 'File' parameter set to auto-correct warnings (#802) #817

bergmeister commented Aug 30, 2017

bergmeister commented Aug 30, 2017

kapilmb commented Sep 8, 2017

kapilmb Sep 8, 2017

bergmeister Sep 8, 2017 •

edited

Loading

kapilmb Sep 8, 2017

bergmeister Sep 8, 2017 •

edited

Loading

kapilmb Sep 12, 2017

kapilmb Sep 8, 2017 •

edited

Loading

bergmeister Sep 12, 2017

bergmeister commented Sep 12, 2017 •

edited

Loading

bergmeister commented Oct 16, 2017

bergmeister commented Nov 3, 2017

SteveL-MSFT commented Nov 3, 2017

adityapatwardhan Nov 6, 2017

adityapatwardhan Nov 6, 2017

adityapatwardhan Nov 6, 2017

bergmeister commented Nov 6, 2017 •

edited

Loading

SteveL-MSFT commented Nov 6, 2017

bergmeister commented Nov 6, 2017 •

edited

Loading

kapilmb commented Nov 8, 2017

bergmeister commented Nov 9, 2017 •

edited

Loading

kapilmb commented Nov 15, 2017

bergmeister commented Dec 2, 2017

JamesLear92 commented Apr 6, 2018

bergmeister commented Apr 6, 2018 •

edited

Loading

Add Fix switch parameter for 'File' parameter set to auto-correct warnings (#802) #817

Add Fix switch parameter for 'File' parameter set to auto-correct warnings (#802) #817

Conversation

bergmeister commented Aug 30, 2017

bergmeister commented Aug 30, 2017

kapilmb commented Sep 8, 2017

kapilmb Sep 8, 2017

Choose a reason for hiding this comment

bergmeister Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

kapilmb Sep 8, 2017

Choose a reason for hiding this comment

bergmeister Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

kapilmb Sep 12, 2017

Choose a reason for hiding this comment

kapilmb Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

bergmeister Sep 12, 2017

Choose a reason for hiding this comment

bergmeister commented Sep 12, 2017 • edited Loading

bergmeister commented Oct 16, 2017

bergmeister commented Nov 3, 2017

SteveL-MSFT commented Nov 3, 2017

adityapatwardhan Nov 6, 2017

Choose a reason for hiding this comment

adityapatwardhan Nov 6, 2017

Choose a reason for hiding this comment

adityapatwardhan Nov 6, 2017

Choose a reason for hiding this comment

bergmeister commented Nov 6, 2017 • edited Loading

SteveL-MSFT commented Nov 6, 2017

bergmeister commented Nov 6, 2017 • edited Loading

kapilmb commented Nov 8, 2017

bergmeister commented Nov 9, 2017 • edited Loading

kapilmb commented Nov 15, 2017

bergmeister commented Dec 2, 2017

JamesLear92 commented Apr 6, 2018

bergmeister commented Apr 6, 2018 • edited Loading

bergmeister Sep 8, 2017 •

edited

Loading

bergmeister Sep 8, 2017 •

edited

Loading

kapilmb Sep 8, 2017 •

edited

Loading

bergmeister commented Sep 12, 2017 •

edited

Loading

bergmeister commented Nov 6, 2017 •

edited

Loading

bergmeister commented Nov 6, 2017 •

edited

Loading

bergmeister commented Nov 9, 2017 •

edited

Loading

bergmeister commented Apr 6, 2018 •

edited

Loading