GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — ReviewUnusedParameter: Do not trigger when MyInvocation.BoundParameters or PSCmdlet.MyInvocation.BoundParameters is used by jegannathanmaniganadan · Pull Request #1520 · PowerShell/PSScriptAnalyzer

Conversation

@jegannathanmaniganadan

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 PSReviewUnusedParameter has false positive when using $MyInvocation.BoundParameters #1505

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, .ps1 and .psm1 files 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Manigandan Jegannathan and others added 6 commits May 22, 2020 19:34
Manigandan Jegannathan
Enhance ReviewUnusedParameter to bail out if $MyInvocation.BoundParma…
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
d4b2ab8 
Enhance ReviewUnusedParameter to bail out if $MyInvocation.BoundParameters encountered
@ghost

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jegannathanmaniganadan Manigandan Jegannathan (jegannathanmaniganadan) mentioned this pull request Jun 11, 2020
PSReviewUnusedParameter has false positive when using $MyInvocation.BoundParameters #1505
Closed
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jun 11, 2020
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this 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:

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rjmholt and jegannathanmaniganadan reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread Rules/ReviewUnusedParameter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bergmeister
Christoph Bergmeister (bergmeister) requested a review from Rob Holt (rjmholt) June 11, 2020 18:45
@bergmeister Christoph Bergmeister (bergmeister) added Area - Rule Sets Issue - Enhancement labels Jun 11, 2020
rjmholt
Rob Holt (rjmholt) approved these changes Jun 11, 2020
View reviewed changes

@rjmholt Rob Holt (rjmholt) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Rules/ReviewUnusedParameter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Rules/ReviewUnusedParameter.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Manigandan Jegannathan
- Label boolean parameter
7c37cf4 
- Add pester tests
@jegannathanmaniganadan
Manigandan Jegannathan (jegannathanmaniganadan) requested a review from Christoph Bergmeister (bergmeister) June 14, 2020 06:19
@jegannathanmaniganadan

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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bergmeister 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
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Jun 14, 2020
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks, looks good 😊

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bergmeister
Christoph Bergmeister (bergmeister) merged commit 1b4a9b9 into PowerShell:master Jun 14, 2020
@bergmeister Christoph Bergmeister (bergmeister) mentioned this pull request Jun 16, 2020
ReviewUnusedParameter does not capture parameters implicitly used to determine ParameterSetName #1527
Closed
@SydneyhSmith Sydney Smith (SydneyhSmith) mentioned this pull request Aug 25, 2020
PSReviewUnusedParameter generates a warning parameter if we use $MyInvocation.MyCommand #1575
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@jegannathanmaniganadan @rjmholt @bergmeister
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.