Visitar URL original
Combine multiple suppressions applied to the same diagnostic by rjmholt · Pull Request #1699 · PowerShell/PSScriptAnalyzer · GitHub
Skip to content

Combine multiple suppressions applied to the same diagnostic - #1699

Merged
Rob Holt (rjmholt) merged 16 commits into
PowerShell:masterfrom
rjmholt:merge-suppressions
Jul 23, 2021
Merged

Combine multiple suppressions applied to the same diagnostic#1699
Rob Holt (rjmholt) merged 16 commits into
PowerShell:masterfrom
rjmholt:merge-suppressions

Conversation

@rjmholt

@rjmholt Rob Holt (rjmholt) commented Jul 21, 2021

Copy link
Copy Markdown
Contributor

PR Summary

Fixes #1691.

Instead of emitting a suppressed record for each suppression on a diagnostic, we combine all suppressions that apply to a diagnostic into a single object.

This partially reimplements #1694.

/cc t-lipingma

PR Checklist

@rjmholt Rob Holt (rjmholt) mentioned this pull request Jul 21, 2021
Closed
6 tasks
JamesWTruher
James Truher (JamesWTruher) reviewed Jul 21, 2021
View reviewed changes
Comment thread Engine/Helper.cs
Comment thread Engine/Helper.cs
Comment thread Tests/Engine/RuleSuppression.tests.ps1
rjmholt
Rob Holt (rjmholt) commented Jul 21, 2021
View reviewed changes
Comment thread Engine/Helper.cs

for (int i = 0; i < suppressed.Length; i += 1)

// Do any error reporting for misused RuleSuppressionIDs here.

@rjmholt Rob Holt (rjmholt) Jul 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

James Truher (@JamesWTruher) elaborated on error reasoning here

Comment thread Tests/Engine/RuleSuppression.tests.ps1
Comment thread Engine/Helper.cs
@t-lipingma

This comment has been minimized.

Sign in to view
@rjmholt

Rob Holt (rjmholt) commented Jul 22, 2021

Copy link
Copy Markdown
Contributor Author

string may change to String

Our preferred style is to use the language aliases over the full type names. For example string instead of String and int instead of Int32.

bergmeister
Christoph Bergmeister (bergmeister) reviewed Jul 22, 2021
View reviewed changes
Comment thread Engine/Helper.cs
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Jul 22, 2021
View reviewed changes

@bergmeister 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.

Looks good from a high level. I agree this PR is probably preferential to the original one.

@rjmholt

Rob Holt (rjmholt) commented Jul 23, 2021

Copy link
Copy Markdown
Contributor Author

Note that suppression attributes didn't seem to work in PSv4 until I made the change in 498a8c9

@rjmholt
Rob Holt (rjmholt) merged commit 9112135 into PowerShell:master Jul 23, 2021
@rjmholt
Rob Holt (rjmholt) deleted the merge-suppressions branch July 23, 2021 21:54
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
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add a list of suppressions for each dianostic

4 participants

@rjmholt @t-lipingma @bergmeister @JamesWTruher