Add rule for missing process block when command supports the pipeline

[mattmcnabb]

PR Summary

This PR adds a rule to warn when a function has at least one parameter that supports the pipeline but doesn't implement a process block. This feature is detailed in issue #1368.

I still need to add tests and docs, but wanted to get this out there to take any feedback on overall approach or general details such as rule naming, messages, etc.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
• Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
• 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.

[bergmeister]

Awesome, let me know if you need help with writing the test. Is the code for the rule already working and ready for review? In case you encounter issues with the build we are there for you as well, the first goal would be to have at least 1 of the 4 build environments green, I'm happy to help getting the edge cases for other environments right as I've got a bit of experience in it already.
In the meantime, I've re-targeted the PR to the master branch as the development branch is just there for legacy purposes

[mattmcnabb]

@bergmeister yes the code appears to be working as-is - I just didn’t get a chance to write tests yet. I’ll take a look later today to see if there are any compatibility quirks that trip me up, but I think this one is pretty straight-forward. Thanks!

[bergmeister]

Because you added strings to the .resx files of the rules project the strongly typed classes file needed to be updated to fix the build, I've just pushed a change for that. If you need to change those resource strings again, you can automatically get them re-created by running .\New-StronglyTypedCsFileForResx.ps1 Rules

[bergmeister]

Just some minor comments about the code style that we could improve a bit more but the code looks otherwise quite solid already :-)
Have a look at some existing tests and it should not be too hard to write one, otherwise let me know and I can help

[mattmcnabb]

Because you added strings to the .resx files of the rules project the strongly typed classes file needed to be updated to fix the build, I've just pushed a change for that. If you need to change those resource strings again, you can automatically get them re-created by running .\New-StronglyTypedCsFileForResx.ps1 Rules

@bergmeister thanks for all your input on this! I'm refactoring based on your style suggestions, but after your change in 1ad72e7 I'm getting the error:

Invoke-ScriptAnalyzer : Could not find any resources appropriate for the specified culture or the neutral culture.
Make sure "Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules.Strings.resources" was correctly embedded or
linked into assembly "Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules" at compile time, or that all the
satellite assemblies required are loadable and fully signed.

I'm sure I'm missing somethings, but don't see exactly what it is.

[bergmeister]

Ok, sorry for that, I pushed another commit where I used Visual Studio for the auto-generation, maybe the script doesn't work any more. In the long term view we plan to move to .Net Core 3, which would handle that for us.

[bergmeister]

I also noticed you had a big diff on the whole resx file, so I reverted the change and re-added your entries for a clean diff in that file. Out of interest, which editor are you using?

[mattmcnabb]

I also noticed you had a big diff on the whole resx file, so I reverted the change and re-added your entries for a clean diff in that file. Out of interest, which editor are you using?

using visual studio 2019 - think that was a line endings or encoding issue?

[mattmcnabb]

I think this is ready to merge now. Let me know what you think about the refactor, tests and the help doc. Again, thanks for your help with this - I'm new to the project so getting started was a bit of a hurdle.

EDIT: No it's not - I forgot a test case and it broke some other tests. I'll fix it up straight away.

[mattmcnabb]

Ok, I think that’s all set now. My test are passing, but I’m still seeing several test failures. One test appears to be counting rules, so naturally that failed. 12 others seem to have returned a null reference exception. Not sure about that yet. I’ll leave this be for today and look at it again tomorrow.

[mattmcnabb]

@bergmeister Well I think I got most of the tests working, but now there is one failing in the Helper tests:

Expected the actual value to be less than or equal to 11, because Number of Runspaces should not exceed size of runspace pool cache (10) plus the the default runspace pool (1) in CommandInfoCache, but got 14.

at <ScriptBlock>, C:\projects\psscriptanalyzer\Tests\Engine\Helper.tests.ps1: line 33
33:             (Get-Runspace).Count | Should -BeLessOrEqual 11 -Because 'Number of Runspaces should not exceed size of runspace pool cache (10) plus the the default runspace pool (1) in CommandInfoCache'

I'm not sure I can troubleshoot that one, otherwise I think this is ready. Thanks!

[bergmeister]

That sounds great. You can ignore that one test about runspaces, it is a known, new flaky test, for which we already have a fix in another PR. Thanks so far, I'll try to review the recent changes in the next days

[mattmcnabb]

@bergmeister Should I be worried that the rule doesn't highlight the parameter in VSCode:

I removed any installations I had of the module and copied the built module into my module path. It appears to be highlighting the existing variable rule, but not mine. The rule does trip on Invoke-ScriptAnalyzer though.

[bergmeister]

In VS-Code not all rules are turned on by default, you can do that either by pointing it to a PSSA settings file or for your purposes just use the PowerShell: Select ScriptAnalyzer Rules command, which can enable other rules (but the choice of settings does not persist, i.e. goes back to the default after you close VS-Code)

I made the diff clean again, but I cannot see a difference in encoding or newlines, maybe you have a non-standard VS or git setting. Getting the diff clean is important here because otherwise other people would get an immediate merge conflict in their existing PRs that is not even easy to resolve.

[bergmeister]

I am happy, just a few ideas for tweaking the user facing messages. I assigned 2 members of the PS team for the final sign off

[mattmcnabb]

I made the diff clean again, but I cannot see a difference in encoding or newlines, maybe you have a non-standard VS or git setting. Getting the diff clean is important here because otherwise other people would get an immediate merge conflict in their existing PRs that is not even easy to resolve.

Yeah, not sure what's going on there, but it must be git because it looked fine until I pushed! I'm ok with the suggestions, so I am using Github's code review commit to apply these.

[mattmcnabb]

@rjmholt @bergmeister just checking in to see if there is anything else you need from me on this PR? Thanks!

[bergmeister]

From my side it is a yes. If Rob is happy as well with the recent changes that he requested, then I'd merge it.

[rjmholt]

LGTM!