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

PSReviewUnusedParameter: Add CommandsToTraverse option #1921

Merged
merged 5 commits into from
Feb 13, 2024
Merged

PSReviewUnusedParameter: Add CommandsToTraverse option #1921

merged 5 commits into from
Feb 13, 2024

Conversation

FriedrichWeinmann
Copy link
Contributor

@FriedrichWeinmann FriedrichWeinmann commented Jun 1, 2023

PR Summary

Proposed resolution of the issue of PSReviewUnusedParameter not traversing into scriptblocks of common commands such as Where-Object or ForEach-Object (#1472).

Implementation:

  • Added a new rule setting: traverseList (string[]). Scriptblocks provided as parameter to one of the listed commands will also be checked for variables.
  • Added Where-Object and ForEach-Object explicitly to the list of commands to traverse

Notes & Thoughts

This is currently not too refined, but works for what it does.
Does not address $using use in Invoke-Command or other edge cases (e.g. ForEach-Object -Parallel).

But it does solve the problem for the most common everyday usage and allows extensibility for people with custom needs (e.g. I'm going to explicitly include Invoke-PSFProtectedCommand).

Also does not cover special cases, such as calculated properties of Select-Object. Or scriptblocks stored in variables and later used as argument for commands that are whitelisted. Maybe more detailed configuration options needed for cases like that.

PR Checklist

Explicitly included Where-Object and ForEach-Object scriptblocks to also be searched for variable use
Copy link
Contributor

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

we're definitely going to need new validation for this behavior.
Please create tests for this.

@bergmeister
Copy link
Collaborator

@FriedrichWeinmann We plan to release later this month so if you could add some tests, we could potentially merge and ship this in time?

@FriedrichWeinmann
Copy link
Contributor Author

Sorry about the delay, added tests

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.

@FriedrichWeinmann Thanks for adding tests, whilst reviewing I just noticed that we need to document this new option.

Rules/ReviewUnusedParameter.cs Outdated Show resolved Hide resolved
@bergmeister bergmeister changed the title PSReviewUnusedParameter: Added command traversal option PSReviewUnusedParameter: Add CommandsToTraverse option Feb 6, 2024
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, LGTM now.

@bergmeister bergmeister enabled auto-merge (squash) February 12, 2024 18:10
Copy link
Contributor

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

looks great - thanks for adding the tests

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.

3 participants