FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ReviewUnusedParameter: Do not trigger when MyInvocation.BoundParameters or PSCmdlet.MyInvocation.BoundParameters is used by jegannathanmaniganadan · Pull Request #1520 · PowerShell/PSScriptAnalyzer · GitHub

/ PSScriptAnalyzer Public

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

Merged
Christoph Bergmeister (bergmeister) merged 7 commits into
PowerShell:masterfrom
jegannathanmaniganadan:master
Jun 14, 2020
ConversationCommits (7)ChecksFiles changed
Merged

ReviewUnusedParameter: Do not trigger when MyInvocation.BoundParameters or PSCmdlet.MyInvocation.BoundParameters is used#1520
Christoph Bergmeister (bergmeister) merged 7 commits into
PowerShell:masterfrom
jegannathanmaniganadan:master

Conversation

Manigandan Jegannathan (jegannathanmaniganadan) commented Jun 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

PR Checklist

Deleted user (ghost) commented Jun 11, 2020 •
edited by ghost
Loading

Copy link
Copy Markdown


All CLA requirements met.

Closed
Christoph Bergmeister (bergmeister) reviewed Jun 11, 2020
View reviewed changes

Christoph Bergmeister (bergmeister) left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Rules/ReviewUnusedParameter.cs Outdated
Rob Holt (rjmholt) approved these changes Jun 11, 2020
View reviewed changes

Rob Holt (rjmholt) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, with two small comments

Comment thread Rules/ReviewUnusedParameter.cs Outdated
Comment thread Rules/ReviewUnusedParameter.cs
- Label boolean parameter
7c37cf4 
- Add pester tests

Manigandan Jegannathan (jegannathanmaniganadan) commented Jun 14, 2020

Copy link
Copy Markdown
Contributor Author

Thanks both for your feedback. Christoph Bergmeister (@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 :)

Christoph Bergmeister (bergmeister) changed the title ReviewUnusedParameter Bug Fix ReviewUnusedParameter: Do not trigger when MyInvocation.BoundParameters or PSCmdlet.MyInvocation.BoundParameters is used Jun 14, 2020
Christoph Bergmeister (bergmeister) approved these changes Jun 14, 2020
View reviewed changes

Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, looks good 😊

Christoph Bergmeister (bergmeister) merged commit 1b4a9b9 into PowerShell:master Jun 14, 2020
Closed
Sydney Smith (SydneyhSmith) mentioned this pull request Aug 25, 2020
Closed
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Christoph Bergmeister (bergmeister) bergmeister approved these changes
+1 more reviewer
Rob Holt (rjmholt) rjmholt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area - Rule Sets Issue - Enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PSReviewUnusedParameter has false positive when using $MyInvocation.BoundParameters

3 participants

Back | FazBrowse Home | New Git URL