| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Thanks Matěj Kafka (@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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Ugh sorry GitHub UI see comment I just left in "Conversation" tab of PR. Thanks!
Sorry, something went wrong.
|
Andy Jordan (@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? |
Sorry, something went wrong.
Christoph Bergmeister (@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. |
Sorry, something went wrong.
|
Matěj Kafka (@MatejKafka) I chatted with Patrick Meinecke (@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. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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. :) |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Not sure I follow – are you referring to changing -ReportExit at all, or the extra changes on top of it? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Makes sense, I'll move the follow-up commits to separate PRs, thanks. :) |
Sorry, something went wrong.
There was a problem hiding this comment.
Fantastic, thank you!
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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