Skip to content
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

Conversation

jegannathanmaniganadan
Copy link
Contributor

@jegannathanmaniganadan jegannathanmaniganadan commented Jun 11, 2020

PR Summary

PR Checklist

@ghost
Copy link

ghost commented Jun 11, 2020

CLA assistant check
All CLA requirements met.

Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with two small comments

Rules/ReviewUnusedParameter.cs Outdated Show resolved Hide resolved
Rules/ReviewUnusedParameter.cs Show resolved Hide resolved
- Add pester tests
@jegannathanmaniganadan
Copy link
Contributor Author

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
Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PSReviewUnusedParameter has false positive when using $MyInvocation.BoundParameters
3 participants