Get PSScriptAnalyzer clean again #180
Merged
+330
−128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HowardWolosky
added
the
technical debt
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Somehow a number of PSScriptAnalyzer issues snuck in. This fixes them.
The main PSScriptAnalyzer issues needing to be fixed were:
* `PSReviewUnusedParameter` - This one came up a lot due to our heavy
usage of `Resolve-RepositoryElements` and `Resolve-ParameterWithDefaultConfigurationValue`
which both end up referencing their parameters by grabbing them off
the stack. That means that `NoStatus` and `Uri` are frequently
never directly referenced. So, exceptions were added. There were
two cases (in GitHubAnalytics) where there was a false positive due
to PowerShell/PSScriptAnalyzer#1472
* `PSUseProcessBlockForPipelineCommand` - We had a number of functions
that took pipeline input, but didn't actuall use the `process` block.
This actually caught a bug with `Group-GitHubIssue` and
`Group-GitHubPullRequest`. Added correct `process` block usage for
most of the functions, but removed pipeline support for those where
it didn't actually make sense anymore.
* `PSUseDeclaredVarsMoreThanAssignments` - These are false positives
in the Pester tests due to the usage of `BeforeAll`. There wasn't
an obvious way to use `SuppressMessageAttribute` in the Pester test,
so I used a hacky workaround to "use" the variable in the `BeforeAll`
block. I could have added the suppression to the top of the file,
but I still want to catch real issues in those files later.
* `PSAvoidOverwritingBuiltInCmdlets` - It turns out that there's a bug
with PSDesiredStateConfiguration in PS Core 6.1.0 where it was exporting
internal functions. This was thus a false-postive flag for Write-Log.
See PowerShell/PowerShell#7209 for more info.
Also, it turns out that `Group-GitHubPullRequest` hadn't actually been
exported, so I fixed that too.
HowardWolosky
force-pushed
the
scriptAnalyzer
branch
from
1a9b8ca to
cd108c8
Compare
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Somehow a number of PSScriptAnalyzer issues have snuck in over time. This fixes them all.
The main PSScriptAnalyzer issues needing to be fixed were:
PSReviewUnusedParameter- This one came up a lot due to our heavyusage of
Resolve-RepositoryElementsandResolve-ParameterWithDefaultConfigurationValuewhich both end up referencing their parameters by grabbing them off
the stack. That means that
NoStatusandUriare frequentlynever directly referenced. So, exceptions were added. There were
two cases (in GitHubAnalytics) where there was a false positive due
to ReviewUnusedParameter does not capture parameter usage within a scriptblock PowerShell/PSScriptAnalyzer#1472
PSUseProcessBlockForPipelineCommand- We had a number of functionsthat took pipeline input, but didn't actuall use the
processblock.This actually caught a bug with
Group-GitHubIssueandGroup-GitHubPullRequest. Added correctprocessblock usage formost of the functions, but removed pipeline support for those where
it didn't actually make sense anymore.
PSUseDeclaredVarsMoreThanAssignments- These are false positivesin the Pester tests due to the usage of
BeforeAll. There wasn'tan obvious way to use
SuppressMessageAttributein the Pester test,so I used a hacky workaround to "use" the variable in the
BeforeAllblock. I could have added the suppression to the top of the file,
but I still want to catch real issues in those files later.
PSAvoidOverwritingBuiltInCmdlets- It turns out that there's a bugwith PSDesiredStateConfiguration in PS Core 6.1.0 where it was exporting
internal functions. This was thus a false-postive flag for Write-Log.
See PSDesiredStateConfiguration exporting internal functions PowerShell/PowerShell#7209 for more info.
Also, it turns out that
Group-GitHubPullRequesthadn't actually beenexported, so I fixed that too.