Skip to content

/ PSScriptAnalyzer

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReviewUnusedParameter: Do not trigger when MyInvocation.BoundParameters or PSCmdlet.MyInvocation.BoundParameters is used #1520

Merged
merged 7 commits into from Jun 14, 2020
Merged

ReviewUnusedParameter: Do not trigger when MyInvocation.BoundParameters or PSCmdlet.MyInvocation.BoundParameters is used #1520

merged 7 commits into from Jun 14, 2020

Conversation

@jegannathanmaniganadan
Copy link
Contributor

jegannathanmaniganadan commented Jun 11, 2020 •
edited

PR Summary

  • ReviewUnusedParameter returns false positives when parameters are splatted using $MyInvocation.BoundParameters & $PSCmldet.MyInvocation.BoundParameters variable. As of now it bails out only for $PSBoundParameter. This fix extends that logic to other bound parameter references.
  • fix #1505

PR Checklist
Manigandan Jegannathan and others added 6 commits May 22, 2020
Manigandan Jegannathan
Enhance ReviewUnusedParameter to bail out if $MyInvocation.BoundParma…
Loading status checks…
67d1620 
…eters encountered
Manigandan Jegannathan
Account for $PSCmdlet.MyInvocation.BoundParameters
02bb9d0
Manigandan Jegannathan
Optmised the way we find bound parameters reference with in script block
1720e71
Manigandan Jegannathan
Address review feedback
f512390
Manigandan Jegannathan
Remove unnecessary casting
09c0cf7
@jegannathanmaniganadan
Merge pull request #1 from jegannathanmaniganadan/dev
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
d4b2ab8 
Enhance ReviewUnusedParameter to bail out if $MyInvocation.BoundParameters encountered
@microsoft-cla
Copy link

microsoft-cla bot commented Jun 11, 2020 •
edited

CLA assistant check
All CLA requirements met.
@jegannathanmaniganadan jegannathanmaniganadan mentioned this pull request Jun 11, 2020
Closed
@bergmeister
bergmeister reviewed Jun 11, 2020
View changes
Copy link
Collaborator

bergmeister left a comment •
edited

Thanks. Implementation looks good to me, just a minor suggestion.
Every new feature or fix needs to be accompanied by one or more test cases though. If you look qt the existing tests, it should be very easy to add 2 tests for the cases that you added:

PSScriptAnalyzer/Tests/Rules/ReviewUnusedParameter.tests.ps1

Lines 53 to 57 in 67805a1

It "has no violations when using PSBoundParameters" {
$ScriptDefinition = 'function Bound { param ($Param1) Get-Foo @PSBoundParameters }'
$Violations = Invoke-ScriptAnalyzer -ScriptDefinition $ScriptDefinition -IncludeRule $RuleName
$Violations.Count | Should -Be 0
}

Also, when you use a closing keyword in the PR description it will close the referenced issue
https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
Rules/ReviewUnusedParameter.cs Outdated Show resolved Hide resolved
@bergmeister bergmeister requested a review from rjmholt Jun 11, 2020
@rjmholt
rjmholt approved these changes Jun 11, 2020
View changes
Copy link
Member

rjmholt left a comment

LGTM, with two small comments
Rules/ReviewUnusedParameter.cs Outdated Show resolved Hide resolved
Rules/ReviewUnusedParameter.cs Show resolved Hide resolved
Manigandan Jegannathan
- Label boolean parameter
Loading status checks…
7c37cf4 
- Add pester tests
@jegannathanmaniganadan
Copy link
Contributor Author

jegannathanmaniganadan commented Jun 14, 2020

Thanks both for your feedback. @bergmeister did not approve the PR, so requested review again.

Every new feature or fix needs to be accompanied by one or more test cases though. If you look qt the existing tests, it should be very easy to add 2 tests for the cases that you added:

Writing pester tests for testing PSSA rule behavior is the most easiest job I have come across :)
@bergmeister bergmeister changed the title ReviewUnusedParameter Bug Fix ReviewUnusedParameter: Do not trigger when MyInvocation.BoundParameters or PSCmdlet.MyInvocation.BoundParameters is used Jun 14, 2020
@bergmeister
bergmeister approved these changes Jun 14, 2020
View changes
Copy link
Collaborator

bergmeister left a comment

Thanks, looks good 😊
@bergmeister bergmeister merged commit 1b4a9b9 into PowerShell:master Jun 14, 2020
3 checks passed
3 checks passed
PSScriptAnalyzer-CI #20200613.4 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@bergmeister bergmeister mentioned this pull request Jun 16, 2020
Closed
@SydneyhSmith SydneyhSmith mentioned this pull request Aug 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmholt rjmholt

@bergmeister bergmeister

Assignees
No one assigned
Labels
Area - Rule Sets Issue - Enhancement
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

PSReviewUnusedParameter has false positive when using $MyInvocation.BoundParameters
3 participants
@jegannathanmaniganadan @rjmholt @bergmeister
You can’t perform that action at this time.