explore improvements to output (especially colors)

[kadoban]

Trying to find ways to improve output of stack in various ways/places, especially using $prettyWarn and $prettyError and etc.

Very open to any comments and suggestions, even if it's just that certain things are a bad idea. Never used any of the Wadler/whatever prettyprinters before at all, so I'm pretty much just making it up as I go along.

related to issue #2650

p.s. Let me know if this shouldn't be a PR until it's further along, I'm fine with deleting it and just having a branch.

• Any changes that could be relevant to users have been recorded in the ChangeLog.md
• The documentation has been updated, if necessary.

[mgsloan]

I think I'd prefer if this was pluralized, wordDocs, since it returns multiple docs. As is, it looks like it might be treating the input as a single word.

[mgsloan]

How about

    $prettyErrorL
      [ flow "stack does not manage installations in global locations."
      , flow "The only global mutation stack performs is executable copying."
      , flow "For the default executable destination, please run"
      , shellColor "stack path --local-bin"
      ]

Where there'd be a new utility

flow :: String -> Doc a
flow = fillSep . wordDoc

No need to use fromString explicitly since OverloadedStrings is on. I think it'd be good if most of the larger logging looked like this, because it's clear how to add some new Doc to the middle of it. Whereas if concatMap flow is used, things need to be refactored for it to be modified.

So, generally, when feasible I think $prettyErrorL should be directly applied to a list, though there are certainly cases where something more complicated makes sense.

[mgsloan]

Looks pretty good thanks for working on this! Good idea to check on it before re-applying the same pattern elsewhere. I'm fine doing this incrementally, may make sense to do it that way to avoid needing to review a huge change in one go.

So, once these changes are ready, makes sense to merge it and then start applying it to more and more of the code. Awesome!

[kadoban]

@mgsloan Thanks a lot for the comments, those all sound like good suggestions. Will apply soon. Yeah, incremental is definitely a good idea.

[kadoban]

Went through and applied those changes, thanks again.

Trying to come up with a hopefully short list of things to do before finishing this PR and working incrementally on further documentation changes:

• Look for places $pretty* needs to be applied that are substantially different from the already done ones. (to make sure the current interface is flexible enough)
• Figure out TH enough to reduce the duplication in s/S/PrettyPrint.hs with $pretty* functions. I have a feeling this is trivial if anyone knows TH and feels like looking at it. The duplication is obvious, and there's a little TODO in the file marking it.
• Add haddocks, especially for the semantic functions.
• Resolve merge conflicts

Edit: do these later, in a different PR:

Optionally, or maybe look later:

• Explore auto-detecting the terminal width and using it instead of the hard-wired value. Probably will require a dependency on some library.
• Allow configuring the terminal width as well, and figure out how that should work with auto-detection. Should probably work a lot like the should-we-use-color detection.

[mgsloan]

Looks good! A couple more comments on the style.

Look for places $pretty* needs to be applied that are substantially different from the already done ones. (to make sure the current interface is flexible enough)

One thing to look for as you go is further uses of display, or introduction of functions that encourage consistent color use associated with types, like displayTargetPkgId, displayErrorPkgId, etc.

Figure out TH enough to reduce the duplication in s/S/PrettyPrint.hs with $pretty* functions. I have a feeling this is trivial if anyone knows TH and feels like looking at it. The duplication is obvious, and there's a little TODO in the file marking it.

Haven't compiled it, but I think this should work:

logWithLoc :: LogLevel -> ExpQ -> ExpQ
logWithLoc level f = do
    loc <- location
    [e| monadLoggerLog loc "" level <=< f |]

prettyDebugL :: Q Exp
prettyDebugL = logWithLoc LevelDebug [| displayWithColor . fillSep |]

 Explore auto-detecting the terminal width and using it instead of the hard-wired value. Probably will require a dependency on some library.

I was thinking about this too, did a bit of searching and found https://stackoverflow.com/a/12807521/1164871 . Could be worth just having this in a new module instead of depending on ncurses. Maybe depending on ncurses would be ok, depends on how problematic the 3rd party deps are, and other deps.

If this is used by default, it should certainly be possible to override it. Also it makes sense to me to have a min bound and max bound on the width used. No point in laying things out at 30 char width, maybe 72 or 80.

[kadoban]

Thanks, that TH thing appeared to be the right idea. At least it compiles and appears to have the same behavior as before, heh. There's still some repetition, but it doesn't bother me personally anymore.

re: display, Yeah good point, I'll be on the lookout for more display instances to create and more customized display* functions too. I'm sure there's a bunch of each needed.

Hmm, that'd be nice if we don't actually have to add a real dependency for terminal detection, I'll try to explore that.

[kadoban]

So I think the problem I see with https://stackoverflow.com/a/12807521/1164871 is that that appears to be linux-only (or at least POSIX only). So that's not going to cover windows, right? Or does stack run under some kind of emulation thing and it'll magically work anyway?

Assuming not: Trying to find if something similar exists in windows. I see some possible API calls and also some mode CLI tool to query it. I might run into trouble there though, I don't currently actually have a windows machine to even dev/test on :-/

I might just run with POSIX-only and leave it to someone else to slip the windows part in.

[mgsloan]

I might just run with POSIX-only and leave it to someone else to slip the windows part in.

Yeah, since this is just an enhancement of output behavior, I think it's fine to omit on windows.

[kadoban]

Ugh, I got tired of trying to figure out if cc-by-sa (what stackoverflow stuff seems to be licensed under) is compatible with BSD3. I'm just going to avoid reading that link any closer, wait a couple of days to forget it and then write my own version based on the ioctl docs and the usual FFI stuff.

In the meantime I'll try to mess with the parts needed surrounding that.

[mgsloan]

I think it makes sense to get this passing CI and merged soon, then continue with additional changes.

Hmm, yeah seems like the opinions on cc-by-sa vary.

• https://developers.slashdot.org/story/16/01/15/1437231/use-code-from-stack-overflow-you-must-provide-attribution suggests that you just need to link to where you got the code

• https://meta.stackexchange.com/a/285783 seems to suggest that if https://stackoverflow.com/users/98117/hammar updates his profile to allow relicensing under BSD3 / MIT, it is allowed.

Sucks, it puzzles me that stackoverflow would have such a restrictive license.

[kadoban]

Yeah, planning on finishing up the last few things in the next couple of days (week at most). Or should I just clean up the current stuff, get it mergeable and do a PR for terminal detection separately?

[mgsloan]

Up to you, but I think earlier is better than later to avoid merge conflicts. Inclined to do terminal detection separately.

[kadoban]

That works for me. Tonight, I'll add some docs to PrettyPrint and clean up with a rebase. Does sound like a better idea than doing everything in this PR.

[kadoban]

I think I'm done mucking about in this PR unless the CI fails or anyone sees any problems. I'll fire up another PR later for incrementally adding more prettyprinting in more places and for autodetection of terminal width and etc. (unless someone else does of course).

[mgsloan]

Awesome, thanks!