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

Combine multiple suppressions applied to the same diagnostic #1699

Merged
merged 16 commits into from Jul 23, 2021

Conversation

rjmholt
Copy link
Contributor

@rjmholt rjmholt commented Jul 21, 2021

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

Engine/Helper.cs Show resolved Hide resolved
Engine/Helper.cs Show resolved Hide resolved
Tests/Engine/RuleSuppression.tests.ps1 Show resolved Hide resolved

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

// Do any error reporting for misused RuleSuppressionIDs here.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JamesWTruher elaborated on error reasoning here

Tests/Engine/RuleSuppression.tests.ps1 Show resolved Hide resolved
Engine/Helper.cs Show resolved Hide resolved
@t-lipingma

This comment has been minimized.

@rjmholt
Copy link
Contributor Author

rjmholt commented Jul 22, 2021

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.

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.

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

@rjmholt
Copy link
Contributor Author

rjmholt commented Jul 23, 2021

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

@rjmholt rjmholt merged commit 9112135 into PowerShell:master Jul 23, 2021
@rjmholt 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a list of suppressions for each dianostic
4 participants