marking code in error messages with backticks#2079
marking code in error messages with backticks#2079paf31 merged 6 commits intopurescript:0.9from soupi:0.9
Conversation
|
This looks good to me, but I feel like regular quote characters (single or double) would be more suitable than backticks, and would match some of our other errors. |
|
Alright. I'll make the change tonight. |
|
Are you going with double quotes or single quotes? I think single quotes could be a problem since they can also be part of an identifier. |
|
Good point. Maybe backticks is best actually, as we can also have quoted labels for records and accessors. |
|
Yes, I initially chose backticks because |
|
Hmmm, can't we have backticks in the quoted code too, when functions as operators? I didn't have the code snippet in the output when I used the deprecated syntax for operator sections to try to get a good example, but I think there will always be a few cases where we will have wrong quotes. I think colored output might be the best solution, but we don't do that anywhere else yet, right? |
|
Another solution is to put code in an indented block, but that could lead to long errors. |
|
@soupi Sorry, but this will need rebasing now due to other changes that went in. I may have made your job easier now though, as most types of name go through one function in the error printing. |
|
Thanks for rebasing, there's one thing that will need a little tweak though - we probably want to move uses of |
|
Ah, all right. I'll make the necessary changes :) |
|
Does this work if something spans multiple lines? |
|
it does something like: |
hconcat 😆 |
|
We have a combinator to put the final tick in the right place somewhere in |
|
Boxes should now look like this: I have also renamed |
|
I don't know about this. I do appreciate all the work here, and the appearance really shouldn't matter, but the backticks just look .. odd. I'd rather go with coloring the types in the output, but then we'd need to modify the |
|
I'm not sure I'm satisfied with how it looks either and I think colored types will look better as well. Being able to immediately focus on the types is nice. Perhaps we should close this for now and open an issue about colored types instead? Or maybe ask in the #2056 |
|
I think it's #1517. |
|
I agree with @paf31, yeah. Looking back at the original issue, as far as I can tell, the only ones that have a risk of causing confusion are "unknown type X" and "unknown value X" (imagine eg X is Error, but I guess there probably are others). What about quoting just those? |
|
I can do that. should I do that? |
|
In that case, it's probably just |
|
Yes please 😀 Sent from my iPhone
|
|
how about putting ansi colors in the regular |
|
We could, but I think it would be a bit cleaner to just not put them there in the first place. |
|
I've only added the yellow color around code. the rest are from the haskell testing framework I think :) Note that I've also taken the liberty to choose whether some places will report errors with color or not, but I'm not very familiar with the compiler so I might have made some mistakes there as well. |
purescript.cabal
Outdated
| utf8-string >= 1 && < 2, | ||
| vector -any | ||
| vector -any, | ||
| ansi-terminal >= 0.6.2 && < 0.7 |
There was a problem hiding this comment.
Could you please re-sort these?
psc/Main.hs
Outdated
| printWarningsAndErrors verbose False warnings errors = do | ||
| when (P.nonEmpty warnings) $ | ||
| hPutStrLn stderr (P.prettyPrintMultipleWarnings verbose warnings) | ||
| hPutStrLn stderr (P.prettyPrintMultipleWarnings True verbose warnings) |
There was a problem hiding this comment.
Do we need to check if the output is a terminal?
|
Purely aesthetic: make sure to test in terminal with a light theme. I recall yellow working at work though. |
|
Hmm, good point. Perhaps an alternative is something like bold text, or underlined, or swapping the background and foreground colors. Or just picking a different color, if necessary. |
|
Worst case, we can provide a |
|
Perhaps the |
psc/Main.hs
Outdated
| cc <- bool Nothing (Just P.defaultCodeColor) <$> ANSI.hSupportsANSI stderr | ||
| when (P.nonEmpty warnings) $ | ||
| hPutStrLn stderr (P.prettyPrintMultipleWarnings (P.defaultPPEOptions { P.ppeFull = verbose }) warnings) | ||
| hPutStrLn stderr (P.prettyPrintMultipleWarnings (P.defaultPPEOptions { P.ppeCodeColor = cc, P.ppeFull = verbose }) warnings) |
There was a problem hiding this comment.
Let's pull the options out into a let binding here.
|
Thanks! |
|
What about just dimming the color? |
|
It should be dull yellow now. |
* marking code in error messages with backticks * adding 'endWith' box combinator and using it to mark code on boxes * removing backticks from multiple lines * marking code with color * refactoring prettyPrintSingleError to get an Options record. namely - PPEOptions * Checking for terminal

as discussed on #2056 and #2060 :)