Fail the tool if any analysis fails #200
Conversation
| m.Code, | ||
| m.Message | ||
| ) | ||
| ) | ||
|
|
||
| () | ||
|
|
||
| let writeReport (results: AnalyzerMessage list option) (codeRoot: string option) (report: string) = | ||
| let writeReport (results: AnalyzerMessage list) (codeRoot: string option) (report: string) = |
There was a problem hiding this comment.
This was never actually optional; we always statically knew that this option was Some. I tightened up the domain by representing that fact in the types.
| ) | ||
| |> Async.Parallel | ||
|
|
||
| return | ||
| Some |
There was a problem hiding this comment.
There was never any chance that this was None, so again I tightened up the types to get rid of the option. Instead it's now applied where it's needed (namely in the big match in Program.fs).
Smaug123
force-pushed
the
fail-on-failure-2
branch
from
be1039c to
25ab689
Compare
| ) | ||
| |> Array.map (fun ctx -> | ||
| logger.LogInformation("Running analyzers for {0}", ctx.FileName) | ||
| client.RunAnalyzers ctx | ||
| match ctx with |
There was a problem hiding this comment.
This could all have been done point-free but I thought it was clearer this way.
| let mappedMessages = messages |> List.map (mapMessageToSeverity mappings) | ||
| yield! mappedMessages | ||
| ] | ||
| messagesPerAnalyzer |
There was a problem hiding this comment.
(Again, this could have been point-free.)
| @@ -375,20 +384,6 @@ let writeReport (results: AnalyzerMessage list option) (codeRoot: string option) | |||
| logger.LogError(ex, "Could not write sarif to {report}", report) | |||
| logger.LogInformation("{0}", ex) | |||
|
|
|||
| let calculateExitCode (msgs: AnalyzerMessage list option) : int = | |||
There was a problem hiding this comment.
This is now extracted to the bottom of the file, where the logic is inlined. That's because our exit code now depends on knowledge we only have down there, and I didn't really fancy passing more args into this function.
| @@ -623,6 +618,7 @@ let main argv = | |||
| | [], Some fscArgs -> | |||
| runFscArgs client fscArgs exclInclFiles severityMapping | |||
| |> Async.RunSynchronously | |||
| |> Some | |||
There was a problem hiding this comment.
(Fallout from the "remove the spurious option type" in FSharp.Analyzers.SDK.fs)
There was a problem hiding this comment.
Looks good from afar! I'll let @dawedawe do a proper review as he is more familiar with the latest of the CLI tool (I think).
One thing that would be great if we had some additional help text about the exit codes. On how to interpret them.
|
Personally I'm less concerned about documenting exit codes because we already log on all these error cases. |
Thanks for the work. We can document the exit codes in another PR, then. |
|
Would you mind merging this please? I don't have permission to. |
|
@Smaug123 would you like a new release with all your latest changes? |
|
That would be great, thanks! |
|
Released https://www.nuget.org/packages/FSharp.Analyzers.SDK/0.24.0 (and subsequentially https://www.nuget.org/packages/G-Research.FSharp.Analyzers/0.8.0) |
|
Thanks - working perfectly (https://github.com/Smaug123/fsharp-prattparser/actions/runs/7708627409/job/21008123060?pr=2 now loudly fails rather than silently failing). |
Motivation is https://github.com/Smaug123/fsharp-prattparser/actions/runs/7619221208/job/20751901249 , which succeeded despite the output being as follows:
In order not to have a heap of primitive types like
_ list option, I pulled out the failure case into a single-case DU. That way, the semantics are much clearer if you're only looking at the type signatures.