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

Console Color not reset? #249

Closed
matthid opened this issue May 22, 2018 · 12 comments
Closed

Console Color not reset? #249

matthid opened this issue May 22, 2018 · 12 comments

Comments

@matthid
Copy link

matthid commented May 22, 2018

From time to time I see fsprojects/FAKE#1858 in my shell.
And somehow I feel like expecto is to blame here.

Regarding

let atomicallyWriteColouredTextToConsole sem (parts: ColouredTextPart list) =
lock sem <| fun _ ->
let originalColour = Console.ForegroundColor
let mutable currentColour = originalColour
parts |> List.iter (fun (text, colour) ->
if currentColour <> colour then
Console.ForegroundColor <- colour
currentColour <- colour
Console.Write(text)
)
if currentColour <> originalColour then
Console.ForegroundColor <- originalColour

Is it possible that multiple LiterateConsoleTarget instances exist for example? Or that the execution is cancelled because another test has failed and it doesn't reset the color in that scenario? Is there anything that comes to mind?

@adamchester
Copy link
Contributor

Yes, it's quite likely there are either multiple instances using different semaphore locks, or possibly even just loggers being written to async.

Unfortunately there's no easy way to make the coloured output "atomic" across multiple console writers using these APIs (here, we can only serialise writers that use the same lock object, the sem argument). This is because of the way the Console.ForegroundColor and related APIs work.

A good option to work towards is to move away from the System.Console APIs for colours and instead use ANSI console colour codes. This way, the entire line written to the console TextWriter.WriteLine(string), along with embedded colour codes, is written atomically. Even Windows 10+ and VSCode etc are supporting this now.

However, until that happens, the best we can do is make sure we serialise console writers.

@matthid
Copy link
Author

matthid commented May 22, 2018

Note that if Console.Write throws (don't know if there is a scenario where that happens) or some other exception happens than the console color will not be reset either.

Also note that I'm not really too much concerned about wrong colors within the run. All I currently care about is that expecto should leave my shell in a sane state :/

@haf
Copy link
Owner

haf commented May 23, 2018

See #248 - I think the case is that we're not waiting for the console printer to finish

@matthid
Copy link
Author

matthid commented May 23, 2018

Can I somehow workaround?

@haf
Copy link
Owner

haf commented May 23, 2018

You can change all info etc to infoWithBP in this repo and PR that and I'll release it. Generally I think I need to change https://github.com/logary/logary/blob/master/src/Logary.Facade/Facade.fs#L216 into https://github.com/logary/logary/blob/master/src/Logary/LoggerModule.fs#L33 — or what do you think is the right semantics for Async?

@matthid
Copy link
Author

matthid commented May 23, 2018

I have no idea what you are talking about :)
The only thing I did is search for ForegroundColor and the result looked suspicious. There is no info or infoWithBP in the two links?

@haf
Copy link
Owner

haf commented May 23, 2018

Yes there is in the first link

@matthid
Copy link
Author

matthid commented May 23, 2018

So it will no longer be async basically?

@haf
Copy link
Owner

haf commented May 23, 2018

Yes, it being async means it's not blocking your output (which means in turn it doesn't reset the colour)

@haf
Copy link
Owner

haf commented May 25, 2018

screen shot 2018-05-25 at 11 27 48

This is what the output looks like in Logary proper; we have a different way of handling data there. I think we should have a design session around how to handle modern terminals and how to deal with structured output (and not just structured logging/data of messages).

Also, we should move to ANSI escape sequences, opted out automatically for terminals it's known not to work with.

Here's the PR for the new Facade causiq/logary#335

@AnthonyLloyd
Copy link
Contributor

Yes there are a few console issues that should be addressed together. Would be good to perfect the output.

I'm in the highlands of Scotland next week so may not be so easy to stay in touch.

@AnthonyLloyd
Copy link
Contributor

resolved with #258

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

4 participants