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

Switch to elevate warning to errors #1534

Open
TylerLeonhardt opened this issue Jun 28, 2020 · 6 comments
Open

Switch to elevate warning to errors #1534

TylerLeonhardt opened this issue Jun 28, 2020 · 6 comments

Comments

@TylerLeonhardt
Copy link
Member

Summary of the new feature

As a user, I want a single switch to elevate warnings to errors so that my repos can have error-less, warning-less code. In CI scenarios, like @JustinGrote's Super-Duper-Linter I need the task to fail when there are only warnings.

Proposed technical implementation details (optional)

@rjmholt mentioned the -NoExit param but I'm not sure if that will exit on warnings as well...

What is the latest version of PSScriptAnalyzer at the point of writing

1.19

@ghost ghost added the Needs: Triage 🔍 label Jun 28, 2020
@TylerLeonhardt
Copy link
Member Author

msbuild already has this ability via the csproj

@JustinGrote
Copy link

@TylerLeonhardt fyi the linter will be able to set a fail level (info, warning, error) based on the diagnostic record output so it won't be explicitly needed for the linter but still a nice-to-have.

@bergmeister
Copy link
Collaborator

bergmeister commented Jun 28, 2020

The -EnableExit switch returns an exit code equivalent to the number of diagnostic records, no matter the severity. There is also a Severity parameter to run only rules of one or more severities.
Even after that one could still easily write some logic to return a different exit code based on analysis of the returned DiagnosticRecords.
Therefore I don't understand the ask here. Maybe you could explain your example in more detail please?

@ghost
Copy link

ghost commented Jul 14, 2020

Closing due to inactivity

@ghost ghost closed this as completed Jul 14, 2020
@rjmholt rjmholt reopened this Jul 14, 2020
@rjmholt
Copy link
Contributor

rjmholt commented Jul 14, 2020

I've thought about this some more, and I think it's at least worth continuing to track. My thinking here:

  • -EnableExit and exit 1 are something of an antipattern, since now the command ends the whole program. Not even just the calling script. It can't be caught or otherwise intercepted, it's a process kill switch that doesn't compose well with other PowerShell concepts. The right way to signal a failure I think is to throw or Write-Error and let PowerShell determine the exit code. That ensures that information is provided about the reason for failing
  • Exceptions don't map well to recoverable errors/diagnostics either though -- nobody likes AggregateException
  • Should PSSA be responsible for the error semantics? It emits objects today that allow users to hook up diagnostics as they please. But perhaps we should seek to standardise a common pattern like throwing/writing errors if diagnostics are present
  • Wanting to re-configure individual rules' severities seems logical to me -- it certainly saves us having to debate how severe things are in issues. Don't think this rule is the right severity? Go and set a different one in the configuration!

@clcaldwell
Copy link
Contributor

clcaldwell commented Aug 19, 2020

@rjmholt For your last issue above - I 100% agree that rule severity should be configurable. Looks like #1515 is already tracking this.

I might be misunderstanding, but it looks like OP just wants to essentially do:

If ($Results = (Invoke-ScriptAnalyzer).Where{$_.Severity -eq 'Warning'}) {
    Exit $Results.count
}

It doesn't really seem like the type of thing that needs it's own param, IMO. And in situations where you are using -EnableExit instead of parsing the report, it would make sense to just only include the rules that you explicitly want the build to fail for.

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

No branches or pull requests

6 participants