detect terminal width and use it for pretty-printed output #3395
Conversation
Just does the part we actually care about, the width in columns
|
So far I just have the raw terminal width detection in. It seems to work fine but needs more testing, and obviously I need to put in the parts that actually use it, and some configurability as well. |
|
Hmm, is that hsc2hs error on appveyor a CI problem or an actual problem, anyone know offhand? I think it's just saying that the executable doesn't exist, but I'm not sure the right way to fix that. |
|
LGTM sofar, though I am not really all that experienced with FFI |
|
Thanks! Yeah, this is my first time doing C FFI (done a tad of JS FFI), so there's a lot of finger crossing going on. But it seems pretty painless so far surprisingly. Edit: Heh, I say that and then the appveyor CI still fails :( Oh well, hopefully that's an easy fix later. |
Includes actually querying the terminal width automatically when it's not overriden.
|
This basically seems to be working, yay. Need to clean up and test a decent amount still though. |
src/Stack/Constants.hs
Outdated
| @@ -216,3 +218,9 @@ compilerOptionsCabalFlag Ghcjs = "--ghcjs-options" | |||
|
|
|||
| ghcColorForceFlag :: String | |||
| ghcColorForceFlag = "-fdiagnostics-color=always" | |||
|
|
|||
| minTerminalWidth :: Int | |||
| minTerminalWidth = 20 | |||
There was a problem hiding this comment.
20 is really small. I think 40 might be a better minimum. Reasoning is that the user might resize their terminal.
I think having a max makes sense too, it's unpleasant to read a whole paragraph of text stretched on a line. Maybe 200 chars? Of course, an answer to that would be why doesn't the user resize their terminal. I use XMonad, though, so if I open up a terminal on a 4k display, I probably don't really want paragraphs to be single lines, though that does happen.
There was a problem hiding this comment.
Yeah I wasn't sure what to do for a min. 40 does look more reasonable. I'm indifferent to a maximum personally, but that reasoning seems to make sense, I'll throw it in.
Hm, I just noticed that I'm doing the checking all wrong too, it's only checking the user-supplied one, and it's just ignoring it completely if it's too small. Should probably just clip to the allowed range if it's outside it. I'll redo that part, heh.
stack.cabal
Outdated
| @@ -268,6 +268,7 @@ library | |||
| , store-core >= 0.4 && < 0.5 | |||
| , annotated-wl-pprint | |||
| , file-embed >= 0.0.10 | |||
| build-tools: hsc2hs >= 0.68 | |||
There was a problem hiding this comment.
Is this even correct? It doesn't seem to help the appveyor problem, but maybe it's supposed to be there anyway?
I've never really used build-tools before, and I also can't tell if hsc2hs is part of GHC and thus doesn't need to be listed here or what.
There was a problem hiding this comment.
Hmm, could be that hsc2hs needs to be added to stackage. For now, adding stack install hsc2hs to the travis script should fix the issue. Adding hsc2hs to extra-deps might help, as it would specify a version to install, not 100% sure if that works, it used to not, but that may have been fixed.
There was a problem hiding this comment.
Oh, nice catch, it isn't in stackage. Appveyor seems to be the only one that doesn't like it.
Huh, I'm getting other travis CI errors that look like actual code problems. Wacky that I'm not getting it locally though, I'll have to look into that. Edit: fixed travisCI
- implement a maximum - clip to the range instead of just ignoring invalid values - actually check the detected width too, not just a user-supplied one
Hopefully this will get it built automatically on appveyor when we need it.
We're not actually using it on windows anyway, and it's causing problems on appveyor.
kadoban
force-pushed
the
terminal-detection
branch
from
8fa2bc0 to
c8c4f94
Compare
kadoban
changed the title
|
Once this goes in, I'll make an issue in the tracker for Windows not doing terminal detection, just as a reminder. I figure someone on windows will care enough eventually. |
|
Is this ready to merge then? Serious effort @kadoban! |
|
This looks good to me, is it ready for merge @kadoban ? |
|
Thanks! Yep, as far as I know it's good to go. I'm a bit uncertain about other OSes still, like OSX (no way to test that), but I'm as close to sure it should work as I can be. |
|
I bet it will work fine on osx. Merging! |
Related to #2650, detect the width of the terminal and use it for proper output.
Note: this current implementation is only ever going to work on POSIX. I don't currently have ability to dev/test on windows, though it appears that there are APIs that could give this info if anyone else wants to sub that in.
Testing: It seems to work fine on all of the OSes I have. Unfortunately, that's a pretty unrepresentative set of OSes. I mostly have linux variants and one BSD I tried on and it worked fine. I don't have access to OSX or Windows really.
If anyone can, testing on other OSes would really be good. Windows won't do any terminal detection, but the command-line option should work.
Log output doesn't seem to be messed up by this. If anyone can verify that'd be nice though. I don't use logs really ever, so I'm hoping I looked the right places.
Docs: I couldn't find any places the docs actually need to be updated. The
stack --helpoutput is correct already obviously.