fix #1417 modulehelp false positives by Jawz84 · Pull Request #1418 · PowerShell/PSScriptAnalyzer
PR Summary
Fixes #1417
When looking up module version, if the module is not installed, also check if it is loaded and get module version from loaded module
Add 'AttachAndDebug' to paramBlackList, so it does not throw false positives for DEBUG builds.
Small bugfix for the actual blacklist check, the property 'name' of the $parameter variable needs to be used to check against the blacklist.
I have tested this by running:
# in pwsh.exe .\PSScriptAnalyzer\build.ps1 -clean .\PSScriptAnalyzer\build.ps1 .\PSScriptAnalyzer\build.ps1 -test # no errors
# in powershell.exe .\PSScriptAnalyzer\build.ps1 -clean .\PSScriptAnalyzer\build.ps1 .\PSScriptAnalyzer\build.ps1 -test # no errors
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,.ps1and.psm1files 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.
- If the PR is work in progress, please add the prefix
When looking up module version, if the module is not installed, also check if it is loaded and get module version from loaded module Add 'AttachAndDebug' to paramBlackList, so it does not throw false positives for DEBUG builds. Small bugfix for the actual blacklist check, the property 'name' of the $parameter variable needs to be used to check against the blacklist.
I've re-run that failed job on Ubuntu 18.04 but it failed again with that 1 failure. I've re-run CI on master and had the same result. Sometimes it is upstream image changes that can cause that, therefore this 1 failures is not related to your change,
Oh! I haven't tested on Linux, just pwsh on Windows. Could look into that later
This failure started to happen in master on that specific image as well, therefore not something related to your change, so don't worry
I just realized I made one unnecessary change. Because of the trick with temporarily setting $env:psmodulepath, the module will always appear 'installed'. I'll revert that part.
…ut\ module during testing, the built module will always appear 'installed'.
| $commands = Get-Command -FullyQualifiedModule $ms -CommandType Cmdlet,Function,Workflow # Not alias | ||
| # Because on Windows Powershell we have workflow, we need to include it there, but in pwsh, we can't. This makes sure this works on both. | ||
| $commandTypes = ([System.Enum]::GetNames("System.Management.Automation.CommandTypes") -match "^Cmdlet$|^Function$|^Workflow$") | ||
| $commands = Get-Command -FullyQualifiedModule $ms -CommandType $commandTypes |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ms is just PSScriptAnalyzer in this case if I read correctly and PSSA only exports 3 cmdlets, no function/alias/workflow at all, therefore why not simplify it to just:
$commands = Get-Command -FullyQualifiedModule $ms
I could reproduce the error where it complains about workflows locally and just using the proposed line worked fine as well.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, makes a lot of sense. will do that.
| $commands =$null | ||
| $paramBlackList = @() | ||
| $paramBlackList = @( | ||
| 'AttachAndDebug' # Reason: When building with DEGUG configuration, an additional parameter 'AttachAndDebug' will be added to Invoke-ScriptAnalyzer and Invoke-Formatter, but there is no Help for those, as they are not intended for production usage. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change absolutely makes sense, I could reproduce the failure locally using a debug build. Thanks for taking the time to fix it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time. Good improvements! It's good to have everything work 'out of the box', no matter in which configuration it was built.
Just one small suggestion to minimize/simplify one change but otherwise looks good.
On an unrelated note: I saw your initial change to some of the Get-Module version logic and actually think most of it is not necessary and not even right in all cases and should rather be replaced by something like (Get-Command Invoke-ScriptAnalyzer).Module.Version because only this was will it load the right module into memory and return the correct version. But let's defer that to another PR.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks, fully happy now :-)
@rjmholt There is new test failure in master (which is the one we see here) and it happens only on Ubuntu18 and it is another one of your compatibility tests, shall we disable it on Linux as well since we know that those sporadic failures in your compatibility tests come and go depending on image Ubuntu image updates?