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

Combine multiple suppressions applied to the same diagnostic by rjmholt · Pull Request #1699 · PowerShell/PSScriptAnalyzer · GitHub

/ PSScriptAnalyzer Public

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
ConversationCommits (16)ChecksFiles changed
Merged

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

Conversation

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

Rob Holt (rjmholt) mentioned this pull request Jul 21, 2021
Closed
6 tasks
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
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.

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.

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

James Truher (@JamesWTruher) elaborated on error reasoning here

Comment thread Tests/Engine/RuleSuppression.tests.ps1
Comment thread Engine/Helper.cs

This comment has been minimized.

Sign in to view

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.

Christoph Bergmeister (bergmeister) approved these changes Jul 22, 2021
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

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

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

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

Back | FazBrowse Home | New Git URL