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 in-place suppression instead of disabling the rule (OSOE-528) #18

Closed
BenedekFarkas opened this issue Jan 9, 2023 · 6 comments · Fixed by #19
Closed
Assignees

Comments

@BenedekFarkas
Copy link
Member

BenedekFarkas commented Jan 9, 2023

The PSReviewUnusedParameter is disabled in our current configuration, because it produces false positives when a parameter is only used in a local function, but not in the main script body. See PowerShell/PSScriptAnalyzer#1472.

A recent comment (PowerShell/PSScriptAnalyzer#1472 (comment)) tipped me off to try suppressing those cases in-place, so we can re-enable the rule.

Jira issue

@BenedekFarkas BenedekFarkas self-assigned this Jan 9, 2023
@github-actions github-actions bot changed the title PSReviewUnusedParameter in-place suppression instead of disabling the rule PSReviewUnusedParameter in-place suppression instead of disabling the rule (OSOE-528) Jan 9, 2023
@Piedone
Copy link
Member

Piedone commented Jan 9, 2023

Shouldn't we instead wait for the bug to be fixed? In your PRs I see only suppressions, but no change due to the rule finding an issue. So, while it may catch issues in the future, apparently we don't have those frequently, and now it only necessitates suppressing false alarms.

@BenedekFarkas
Copy link
Member Author

The issue has been open for 2 and a half years, so why should we wait more?
There were a handful of true positives I fixed during one of the previous issues.

@Piedone
Copy link
Member

Piedone commented Jan 9, 2023

This rule currently brings more suppressions than fixes. So I don't see why we'd use it, especially since the bugs it guards against are minor.

@Piedone
Copy link
Member

Piedone commented Jan 9, 2023

@DAud-IcI please see this discussion.

@sarahelsaig
Copy link
Member

Ah, I reviewed this a few hours earlier but then took a nap because the merging seemed complex. Shouldn't have slacked if I knew it leaves time for discussion. 😁
Anyway, it's pretty inconsequential either way, since only 2 scripts had to be suppressed.

@Piedone
Copy link
Member

Piedone commented Jan 9, 2023

image

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