Fix table to refer to existing md files, add col for Configurable

[rkeithhill]

PR Summary

As per title and cleanup leftovers in code from deprecated rules AvoidUsingFilePath, AvoidUninitializedVariable and AvoidTrapStatement and PSAvoidUninitializedVariable
Closes #1001

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

• PR has a meaningful title
  • [NA] Use the present tense and imperative mood when describing your changes
• Summarized changes
• [NA] User facing documentation needed
• Change is not breaking
• Make sure you've added a new test if existing tests do not effectively test the code changed
• This PR is ready to merge and is not work in progress
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Fix #887

[bergmeister]

The reason why this file got removed in PR #918 because the table of rules was completely out of-date of date and not offering more information than Get-ScriptAnalyzer returns. Since there is already so much stuff to update (and forget) when adding/updating rules, we decided to keep the maintenance burden low. But your provided information is still useful, maybe it is better to only list the configurable rules here instead?
GitHub also display a merge conflict, I guess you need to get the upstream changes first that delete the file and then re-add it.

[rkeithhill]

By that logic, we wouldn't have this site - https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/enter-pssession?view=powershell-6

I think there is a lot of value in having the documentation be on a website that is linkable (put in blog post, tweets, etc).

BTW totally understandable that there is burden to keeping documentation correct & up-to-date. We could write a script that verifies that A) every link is valid and B) spit out a list of undocumented rules.

[bergmeister]

If you feel strong about this one, then I have no objections to re-add it but then it needs to be fixed with rules (that were missing), severities being checked and link fixes as per original issue #887

[rkeithhill]

I'd like to see it stay. Would you prefer that I add the missing md files in this same PR? Also, looks like you have a build script, I could attempt to implement checking of the rule doc md files in the build script. This is similar to the markdown checking that we do for the PowerShell build.

[bergmeister]

There are other missing md files? I thought it was just the table of contents being out of date.
You could just add another Pester test file here to test the markdown, that would be really great. Unfortunately the PR build integration broke last Friday and I already contacted the MS guys about it, what a shame otherwise you would've seen the multi platform/version build system against PowerShell 4,5,6 on Windows and Linux that I significantly enhanced recently... But as long as Invoke-Pester against the test file works, then it should be fine.

[rkeithhill]

Yup:

05-06 17:38:34 101ms 33> diff $rule $md -SyncWindow 50

InputObject                                SideIndicator
-----------                                -------------
PSAvoidTrapStatement                       =>
PSAvoidUninitializedVariable               =>
PSAvoidUsingFilePath                       =>
PSDscExamplesPresent                       =>
PSDscTestsPresent                          =>
PSReturnCorrectTypesForDSCFunctions        =>
PSStandardDSCFunctionsInResource           =>
PSUseIdenticalMandatoryParametersForDSC    =>
PSUseIdenticalParametersForDSC             =>
PSUseVerboseMessageInDSCResource           =>
PSDSCDscExamplesPresent                    <=
PSDSCDscTestsPresent                       <=
PSDSCReturnCorrectTypesForDSCFunctions     <=
PSDSCUseIdenticalMandatoryParametersForDSC <=
PSDSCUseIdenticalParametersForDSC          <=
PSDSCStandardDSCFunctionsInResource        <=
PSDSCUseVerboseMessageInDSCResource        <=

Some of this is just a slightly different named md file (from the rule). I can fix that. But I see no Trap related rules nor any rules on FilePath.

Is it just me or does PSDSCDscExamplesPresent / PSDSCDscTestsPresent seem a bit redundant? What's with the DSCDsc?

[bergmeister]

OK, I see. I guess the trap rule got deprecated (but not put into the DeprecatedRules folder?), you might need to dig a bit in the history...
Sorry for some of the mess but this is simply what we were already given from the former maintainers.

[rkeithhill]

@bergmeister Can you resolve the merge conflict? It won't let me. I think the conflict arises from the deletion of rule doc README.md file that I've modified.

[bergmeister]

@rkeithhill I pulled upstreamed changes to your branch and just kept the state of your modified file. I use TortoiseGit btw, which was clever enough to offer me the option of either keeping or deleting the file on the conflict.
Thank you for your efforts, I will review this the next days.

[bergmeister]

Overall, the changes to the markdown files are OK but they are missing the references to the new rules PSAvoidTrailingWhitespace, PossibleIncorrectUsageOfRedirectionOperator, PossibleIncorrectUsageOfAssignmentOperator and AvoidAssignmentToAutomaticVariable. I raised the separate issue #1001 to clean up other places where I still see other places referencing the deprecated rules AvoidTrapStatement, AvoidUninitializedVariable and AvoidUsingFilePath in order to keep this PR small and not grow scope.
I took care to adapt our CI scripts to also call your new Documentation test folder and adapt docs, you don't have to worry about it. When I ran them locally, they did not all pass due to the missing links to the new rules. I can take a look at it and it might be a good exercise anyway.

 invoke-pester -Script .\Tests\Documentation\
Executing all tests in '.\Tests\Documentation\'

Executing script C:\Users\cberg\git\rkeithhill_pssa\Tests\Docmentation\RuleDocumentation.tests.ps1

  Describing Validate rule documentation files
    [-] Every rule must have a rule documentation file 311ms
      Expected $null or empty, but got PSPossibleIncorrectUsageOfAssignmentOperator.
      28:         $rulesDocsDiff | Where-Object SideIndicator -eq "<=" | Foreach-Object InputObject | Should -BeNullOrEmpty
      at <ScriptBlock>, C:\Users\cberg\git\rkeithhill_pssa\Tests\Docmentation\RuleDocumentation.tests.ps1: line 28
    [-] Every rule documentation file must have a corresponding rule 101ms
      Expected $null or empty, but got PSPossibleIncorrectUsageOAssignmentOperator.
      31:         $rulesDocsDiff | Where-Object SideIndicator -eq "=>" | Foreach-Object InputObject | Should -BeNullOrEmpty
      at <ScriptBlock>, C:\Users\cberg\git\rkeithhill_pssa\Tests\Docmentation\RuleDocumentation.tests.ps1: line 31
    [-] Every rule must have an entry in the rule documentation README.md file 55ms
      Expected $null or empty, but got @('PSAvoidAssignmentToAutomaticVariable', 'PSAvoidTrailingWhitespace', 'PSPossibleIncorrectUsageOfAssignmentOperator', 'PSPossibleIncorrectUsageOfRedirectionOperator').
      35:         $rulesReadmeDiff | Where-Object SideIndicator -eq "<=" | Foreach-Object InputObject | Should -BeNullOrEmpty
      at <ScriptBlock>, C:\Users\cberg\git\rkeithhill_pssa\Tests\Docmentation\RuleDocumentation.tests.ps1: line 35
    [+] Every entry in the rule documentation README.md file must correspond to a rule 17ms
    [+] Every entry in the rule documentation README.md file must have a valid link to the documentation file 139ms
    [+] Every rule name in the rule documentation README.md file must match the documentation file's basename 113ms
Tests completed in 739ms
Tests Passed: 3, Failed: 3, Skipped: 0, Pending: 0, Inconclusive: 0

[rkeithhill]

Thanks for fixing the typo! If there's anything you need me to do on this PR, let me know. Looking through your previous comments I didn't see any specific asks.

[bergmeister]

What is left is fixing the tests with the new rules (that you previously did not see because the checkout was not up to date) now that the tests fail as expected.
But I can do that, should not be too difficult

[bergmeister]

@rkeithhill I fixed the table with the new rules now but it seems the tests only pass on PowerShell 5.1, but not PowerShell 4 and 6.
After some debugging, it turns out that the PSUseSingularNouns rule but it seems this rule is not compiled for netstandard2.0, see here due to the usage of the PluralizationService API. For PowerShell 4, the PSAvoidGlobalAliases rule is missing due to the usage of StaticParameterBinder.BindCommand.
We would need to tweak the tests for those special scenarios to make CI pass, which runs against PowerShell 4, 5.1 and 6 (Windows and Ubuntu)

[rkeithhill]

Get-ScriptAnalyzerRule should always return the truth for the platform/edition/version it runs on. So we need some way to tweaks the results of getting the list of rule doc filenames and/or entries in the README.md file. I wonder if we could add a new column or two to the README.md where we list platform/edition/version of PowerShell where a rule is available? We could use that information to test properly for such rules on only the supported version/edition. Platform could come into play at some point - maybe a rule is only supported on PSCore on Windows (not Linux/macOS).

Maybe we have something like the .NET Core runtime identifiers for these "special" rules. Perhaps <platform>-<edition>-<version-range> e.g. PSAvoidGlobalAliases would use *-*->=5 and PSUseSingularNouns would use win-desktop-*. If we called this column Availability then the assumption is that an empty entry implies the rule is supported everywhere. Actually having the Availability info would be useful.

BTW I'm just spit-ballin' some ideas here but I would be willing to help implement this.

[bergmeister]

Get-ScriptAnalyzerRule does return the truth for the ps version it is running on, hence why the tests fail for version 4 and 6. The platform does not play a rule, it is just a matter of certain APIs not being available in a given version of PS.
Because it is just 2 rules, I think it is sufficient to just add a note in the respective documentation of the rule and/or the table

[rkeithhill]

OK, just pushed a commit so that all the doc tests now pass.

[bergmeister]

Thanks Keith, I added a note in the 2 rules about their PowerShell version support. If you feel like it, you could add this note to the table but having it at least in the documentation of the rule should be fine.
I also cleaned up some of the leftovers that I found in the code about the deprecated rules that we touched on and fixed some markdown.

[rkeithhill]

add this note to the table

Done. I didn't duplicate the text I just added a superscript/footnote to indicate the rule isn't supported everywhere and to see the doc for details.

[bergmeister]

That's fine but it seems that adding it broke your tests

[rkeithhill]

Yeah, just fixed it according to my local tests. :-)