Set exit code of Invoke-ScriptAnalyzer -EnableExit to total number of diagnostics (#2054)

[MatejKafka]

PR Summary

Fixes #2054 by setting the exit to the total number of emitted diagnostics instead of just the last processed file from the pipeline.

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.

[andyleejordan]

Thanks @MatejKafka! I really like this. Could I ask you to do one small refactor? I checked and RunAnalysis() is returning an IEnumerable which is why you then called ToArray() (well, you could have also used LINQ's Count() extension method, but that's what we're trying to avoid). Instead, since the container of diagnostic records being returned is already a List could you change the return type of RunAnalysis to List? Then you can use the Count property instead of method, and continue to pass it to the next function that takes an IEnumerable (which List satisfies and won't cause any type changes) instead of changing it to take an array.

PSScriptAnalyzer/Engine/Commands/InvokeScriptAnalyzerCommand.cs

Line 439 in 9dacfa8

var diagnostics = new List<DiagnosticRecord>();

[andyleejordan]

Ugh sorry GitHub UI see comment I just left in "Conversation" tab of PR. Thanks!

[MatejKafka]

@andyleejordan Needed to make a few more changes in AnalyzeScriptDefinition, which returned an IEnumerable, which is why did not make this change in the first place, but agree it's the better option. :)

Btw, looking at the code a bit more, imo it would also make sense to move ReportSummary handling to EndProcessing – that way, it's consistent with the new EnableExit behavior, and I'd assume most users want the summary for the whole run, not for each piped file separately. Thoughts?

[andyleejordan]

Btw, looking at the code a bit more, imo it would also make sense to move ReportSummary handling to EndProcessing – that way, it's consistent with the new EnableExit behavior, and I'd assume most users want the summary for the whole run, not for each piped file separately. Thoughts?

@bergmeister thoughts on this? I'm inclined to agree but it seems like a riskier change. Otherwise I'm good with this PR as-is but wanted to ping you before merging.

[andyleejordan]

@MatejKafka I chatted with @SeeminglyScience and confirmed that wouldn't break things, so I'm all for adding that small refactor. It makes sense, then I can do a final review and merge this.

[MatejKafka]

One more thing – currently, the number of violations reported by -ReportSummary does not include parse errors, which is imo a bit weird, especially when the exit code includes them. Would you agree to also including parse errors in the total number of violations and the "Error = ..." print to be consistent?

[MatejKafka]

Updated with changes to -ReportSummary. I tend to make a lot of minor cleanup changes while editing code. If you're not happy about any of the changes, just ping me and I'll remove them from the PR. :)

[andyleejordan]

Ah, yes, sorry. I misunderstood and that moving the report summary logic would be a change on the same order as the exit code change. Need to keep it very minimal.

[MatejKafka]

Not sure I follow – are you referring to changing -ReportExit at all, or the extra changes on top of it?

[andyleejordan]

All the extra changes on top of it. I'm good to merge just commit 04a0149. For the extra report summary changes proposed, let's do it another PR.

[MatejKafka]

Makes sense, I'll move the follow-up commits to separate PRs, thanks. :)

[andyleejordan]

Fantastic, thank you!