Re #4091: Pre-Windows 10, default --color=never #4106
Conversation
mpilgrem
force-pushed
the
colorHelp
branch
3 times, most recently
from
93106db
to
357c386
Compare
|
From a first glance, this looks great. Thank you! It should be fairly quick to review — I'm just searching for a windows reviewer. I suspect we may be better off adding |
| if h == iNVALID_HANDLE_VALUE || h == nullHANDLE | ||
| then return ColorNever -- Invalid handle or no handle | ||
| else do | ||
| tryMode <- try (getConsoleMode h) :: IO (Either SomeException DWORD) |
There was a problem hiding this comment.
I'd recommend using tryAny here: it will make the type signature unnecessary, and be more explicit about using a function that doesn't catch async exceptions.
There was a problem hiding this comment.
Or did you mean to catch only a ConsoleException from below?
There was a problem hiding this comment.
I've now moved away from duplicating ansi-terminal's use of the exception mechanism for this small extract.
| then return ColorAuto -- VT processing already enabled | ||
| else do | ||
| let mode' = mode .|. eNABLE_VIRTUAL_TERMINAL_PROCESSING | ||
| trySetMode <- try (setConsoleMode h mode') :: IO (Either SomeException ()) |
| throwIfFalse action = do | ||
| succeeded <- action | ||
| if not succeeded | ||
| then getLastError >>= throw . ConsoleException |
There was a problem hiding this comment.
Please use throwIO instead of throw. The former is resistant to later refactorings that would cause a bottom value to hide inside something lazy.
stack.yaml
Outdated
| @@ -1,4 +1,4 @@ | |||
| resolver: lts-11.6 | |||
| resolver: lts-11.14 | |||
There was a problem hiding this comment.
Is it strictly necessary to bump the LTS version here? I tend to avoid such updates in unrelated patches.
There was a problem hiding this comment.
Now use an extra-dep instead. (ansi-terminal-0.8.0.4 brings the mintty fix to its hSupportsANSI.)
|
Before turning to the results of the review, an update on testing: I have tested on Windows 7 and 10, both in a native terminal and in a mintty terminal (Cygwin) (the test being |
Implements `--color=never` as default on Windows before Windows 10, if emulation would be required to support ANSI colour codes. Also updates `stack.yaml`, in order to benefit from `ansi-terminal-0.8.0.4`. `defaultColorWhen`, exported by new module `Stack.DefaultColorWhen`, establshes the default. That module differs, pending on whether the operating system is Windows or non-Windows (see the conditional in `package.yaml`). `globalOptsFromMonoid` in module `Stack.Options.GlobalParser` is modified so that the default is an argument rather than a given. A beneficial side-effect of `defaultColorWhen` on Windows is that it enables ANSI on ANSI-capable native terminals. This allows some existing code in module `Main` to be removed.
|
I think my further changes are responsive to all the requested changes. |
Implements
--color=neveras default on Windows before Windows 10, if emulation would be required to support ANSI colour codes.Also bumps the lts-11 resolver in
stack.yamlto lts-11.14, in order to benefit fromansi-terminal-0.8.0.4.defaultColorWhen, exported by new moduleStack.DefaultColorWhen, establishes the default. That module differs, pending on whether the operating system is Windows or non-Windows (see the conditional inpackage.yaml).globalOptsFromMonoidin moduleStack.Options.GlobalParseris modified so that the default is an argument rather than a given.A beneficial side-effect of
defaultColorWhenon Windows is that it enables ANSI on ANSI-capable native terminals. This allows some existing code in moduleMainto be removed.Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.
Tested on Windows 10 and some limited testing on Windows 7.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!