GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Combine multiple suppressions applied to the same diagnostic by rjmholt · Pull Request #1699 · PowerShell/PSScriptAnalyzer

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
  • Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Rob Holt (rjmholt) added 2 commits July 21, 2021 10:05
@rjmholt
Ensure multiply suppressed diagnostics only create one output
05a275d
@rjmholt
Fix suppression with no offset
e486374
@rjmholt
Rob Holt (rjmholt) requested review from James Truher (JamesWTruher) and Christoph Bergmeister (bergmeister) July 21, 2021 17:15
@rjmholt
Defend against NRE
4efd640
@rjmholt Rob Holt (rjmholt) mentioned this pull request Jul 21, 2021
Add some necessary contents for diagnostic suppressions. #1694
Closed
6 tasks
JamesWTruher
James Truher (JamesWTruher) reviewed Jul 21, 2021
View reviewed changes
Comment thread Engine/Helper.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Engine/Helper.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tests/Engine/RuleSuppression.tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Rob Holt (rjmholt) added 4 commits July 21, 2021 12:28
@rjmholt
Complete comment help
2874be3
@rjmholt
Add error emission comment
abdaefd
@rjmholt
Fix justification test
177ed88
@rjmholt
Add extra tests
37a0104
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

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tests/Engine/RuleSuppression.tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Engine/Helper.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@rjmholt
Rob Holt (rjmholt) requested a review from James Truher (JamesWTruher) July 21, 2021 21:28
Rob Holt (rjmholt) added 3 commits July 21, 2021 14:37
@rjmholt
Try error redirection
e4ea36f
@rjmholt
Try to fix errors
5eccd72
@rjmholt
Go back to ErrorAction
200f0ad
@t-lipingma

This comment has been minimized.

Sign in to view
@rjmholt
Fix tests
c942343
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rjmholt
Fix tests
6bfdd25
bergmeister
Christoph Bergmeister (bergmeister) reviewed Jul 22, 2021
View reviewed changes
Comment thread Engine/Helper.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
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

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Rob Holt (rjmholt) added 4 commits July 23, 2021 07:49
@rjmholt
Fix error variable
f1391d3
@rjmholt
Try to filter out other diagnostics
99f146e
@rjmholt
Remove redundant filters
f93ad20
@rjmholt
Use other API for attribute types
498a8c9
@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@rjmholt @t-lipingma @bergmeister @JamesWTruher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.