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
support multiple project parameters in the Cli tool #116
Conversation
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, I'm wondering a bit about the parallel stuff.
src/FSharp.Analyzers.Cli/Program.fs
Outdated
return results |> Option.map (printMessages failOnWarnings) | ||
} | ||
|
||
projects |> List.map runProj |> Async.Sequential |> Async.RunSynchronously |
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.
Any reason we don't want to run these in parallel?
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.
Yes, I'm worried about mangled console output.
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.
Valid concern, although we probably want to be a bit more clever about this eventually.
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.
Can wrap it in a lock/mailboxprocessor.
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.
Maybe eventually ILogger
/ILoggerFactory
support but not sure how much worse that would make binary compatibility issues.
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.
Thanks @TheAngryByrd , I'd prefer to handle that in a separate PR.
src/FSharp.Analyzers.Cli/Program.fs
Outdated
@@ -131,16 +131,23 @@ let printMessages failOnWarnings (msgs: Message list) = | |||
|
|||
msgs | |||
|
|||
let calculateExitCode failOnWarnings (msgs: Message list option) : int = | |||
let calculateExitCode failOnWarnings (msgs: Message list option array) : int = |
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.
Can we not flatten this collection earlier? This type of definition is getting out of hand.
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.
Absolutely, good call
fixes #113