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

Fix colors on Windows (#749) #768

Merged
merged 2 commits into from
Apr 10, 2021
Merged

Fix colors on Windows (#749) #768

merged 2 commits into from
Apr 10, 2021

Conversation

stkb
Copy link
Contributor

@stkb stkb commented Apr 7, 2021

Looking through the Stack source code lead me to the hSupportsANSIWithoutEmulation function. Calling this on Windows 10+ returns Just True but also enables the processing of the Ansi control codes as a side effect.

This change also has the added benefit that (on Windows), the ANSI codes aren't present when output is redirected to a text file.

Code is just proof-of concept; feel free to modify as you wish. Haven't tested on *nix but it's working on Windows for me.

Calling the `hSupportsANSIWithoutEmulation` function on Windows 10+
enables terminal colors where supported.
https://hackage.haskell.org/package/ansi-terminal-0.11/docs/System-Console-ANSI.html#v:hSupportsANSIWithoutEmulation

Also has the added benefit that (on Windows), the ANSI codes aren't
present when output is redirected to a text file.
@stkb
Copy link
Contributor Author

stkb commented Apr 7, 2021

Oh I was just rebasing at the same time you were :) Will wait for CI then look at any errors

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left small suggestions but otherwise looks great! 👏

CHANGELOG.md Outdated Show resolved Hide resolved
src/Spago/RunEnv.hs Outdated Show resolved Hide resolved
@stkb
Copy link
Contributor Author

stkb commented Apr 7, 2021

@f-f Ok as shown by the failing tests, this actually produces a change in behavior. For all platforms now, if the output is directed into a file, the color codes are never included, even without the --no-color flag.

Is this change desired? If not, we can instead just call the hSupportsANSIWithoutEmulation function but ignore the output.

@f-f
Copy link
Member

f-f commented Apr 7, 2021

Good question. It feels like the new behaviour should be more correct, but I don't have a strong opinion on this.
I guess a good call could be to stay consistent with what the compiler does?

@stkb
Copy link
Contributor Author

stkb commented Apr 8, 2021

I can't find any definitive recommendation on this, but yeah my feeling too is that logfiles should be free from the color codes.

After investigating, this is indeed what the purs compiler does (as well as respecting TERM=dumb). However unfortunately PSA doesn't respect either, so output from that is still colored. I guess that's an issue for PSA though...

@f-f
Copy link
Member

f-f commented Apr 8, 2021

Great then, let's move on with this!

@stkb
Copy link
Contributor Author

stkb commented Apr 8, 2021

Yeah I'm just working on making all the tests pass

@stkb
Copy link
Contributor Author

stkb commented Apr 9, 2021

Tests are now passing; I stripped the color codes from the relevant test fixtures.

While we were here I also came across the NO_COLOR spec that is another way to disable colors. It seems to have some critics but was very easy to add support for so I've added that too.

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfect now, great work! 👏👏

@stkb stkb merged commit 06bd975 into purescript:master Apr 10, 2021
@stkb stkb deleted the wincolors branch April 10, 2021 13:21
@garyb
Copy link
Member

garyb commented Apr 10, 2021

Nice work @stkb!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants