Remove stack-specific unicode logic from RIO.Prelude.Logger #73
Conversation
| let text' | ||
| | unicode = text | ||
| | otherwise = T.map replaceUnicode text | ||
| TIO.hPutStr handle' text' |
There was a problem hiding this comment.
This branch is definitely not Stack specific. In the new version of the code, you seem to be ignoring the logUseUtf8 value entirely. This code will not work on any machine with a locale/code page setting that does not support UTF-8.
There was a problem hiding this comment.
If you take a look at the replaceUnicode function, it only replaces two codepoints specific to GHC output.
Yes, logUseUtf8 is ignored entirely, it's intended to be read by the user. On further review, that is probably unexpected and probably should have an accompanying askUseUtf8 or something.
So, what should we do here? There are a few different ways to munge unicode into 7 bit ASCII
There was a problem hiding this comment.
Do other loggers just punt on this? Why is it particularly coming up here? Just that stack has been used in a larger variety of environments?
There was a problem hiding this comment.
I think you're missing the exact line of code that I'm commenting on. The way the code works today is that, if the file handle doesn't support UTF8, we decode the bytestring to Text and then use the textual I/O functions to output it.
There was a problem hiding this comment.
I see. I'm not familiar with how hPutBuilder handle' (builder <> flush) would differ from T.hPutStrLn (decodeUtf8 ...). Seems like it should be pretty equivalent (assuming it parses as utf8).
There was a problem hiding this comment.
Ah, I guess it's because in theory it will work for things that aren't utf-8. Having a hard time finding where in the code that happens, though. I guess the stuff in System.IO does that. It looks like the text io stuff will have different concurrent behavior, potentially interspersing output of chars.
|
Discussed offline. The code is not Stack specific, and needs to remain. There's an argument to be made around the quote replacement, but overall it does good without major harm (just a single traversal of a |
It seems like this should be done differently in stack anyway, directly munging particular process output instead of applying to all of stack's logging.