Skip to content

marking code in error messages with backticks#2079

Merged
paf31 merged 6 commits intopurescript:0.9from
soupi:0.9
May 28, 2016
Merged

marking code in error messages with backticks#2079
paf31 merged 6 commits intopurescript:0.9from
soupi:0.9

Conversation

@soupi
Copy link
Copy Markdown
Contributor

@soupi soupi commented May 1, 2016

as discussed on #2056 and #2060 :)

@paf31 paf31 added this to the 0.9.0 milestone May 1, 2016
@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 1, 2016

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.

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 2, 2016

Alright. I'll make the change tonight.

@jplatte
Copy link
Copy Markdown
Contributor

jplatte commented May 2, 2016

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.

@garyb
Copy link
Copy Markdown
Member

garyb commented May 2, 2016

Good point. Maybe backticks is best actually, as we can also have quoted labels for records and accessors.

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 2, 2016

Yes, I initially chose backticks because ' or " can be used in other contexts :)

@jplatte
Copy link
Copy Markdown
Contributor

jplatte commented May 2, 2016

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?

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 2, 2016

Another solution is to put code in an indented block, but that could lead to long errors.

@garyb
Copy link
Copy Markdown
Member

garyb commented May 12, 2016

@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.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.01%) to 56.038% when pulling a72d4da on soupi:0.9 into b5af366 on purescript:0.9.

@garyb
Copy link
Copy Markdown
Member

garyb commented May 12, 2016

Thanks for rebasing, there's one thing that will need a little tweak though - we probably want to move uses of markCode inside printName, as now it will print things like Unknown value x or `Unknown `type operator (~>).

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 13, 2016

Ah, all right. I'll make the necessary changes :)

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.01%) to 56.038% when pulling 94bdff6 on soupi:0.9 into b5af366 on purescript:0.9.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 13, 2016

Does this work if something spans multiple lines?

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 13, 2016

it does something like:

Error found:
in module `Slides`
at /Users/suppi/code/purescript/purescript-slides/src/Slides.purs line 56, column 27 - line 59, column 3

  Could not match type

    Slides

  with type

    { pos :: Int
    , slides :: Array Slide
    }


while trying to match type `HalogenFP _0 Slides`
  with type `HalogenFP EventSource    `
               { pos :: Int
               , slides :: Array Slide
               }
while checking that expression `(bind (modify (\$1 ->  `
                                                 ...
                                              )
                                      )
                                )
                                (\__unused ->
                                   pure next
                                )
  has type `Free                        `
              (HalogenFP EventSource
                 { pos :: Int
                 , slides :: Array Slide
                 }
                 Move
                 g0
              )
              a1
in value declaration `ui`

where `g0` is a rigid type variable
        bound at line 44, column 1 - line 65, column 1
      `a1` is a rigid type variable
        bound at line 56, column 3 - line 59, column 3
      `_0` is an unknown type

See https://github.com/purescript/purescript/wiki/Error-Code-TypesDoNotUnify for more information,
or to contribute content related to this error.

@natefaubion
Copy link
Copy Markdown
Contributor

`Free                        `

hconcat 😆

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 13, 2016

We have a combinator to put the final tick in the right place somewhere in Pretty.Common.

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 16, 2016

Boxes should now look like this:

tests/TestCompiler.hs:197:
  90) Failing examples 'SuggestComposition.purs' should fail with 'TypesDoNotUnify'
       Error found:
       in module `SuggestComposition`
       at /Users/suppi/code/haskell/purescript/examples/failing/SuggestComposition.purs line 7, column 5 - line 7, column 10

         Could not match type

           Record

         with type

           Function Int


       while trying to match type `{ g :: _0
                                   | _1
                                   }        `
         with type `_2 -> _3`
       while checking that expression `g`
         has type `{ g :: _0
                   | _1
                   }        `
       while checking type of property accessor `g.g`
       in value declaration `f`

       where `_2` is an unknown type
             `_3` is an unknown type
             `_0` is an unknown type
             `_1` is an unknown type

       See https://github.com/purescript/purescript/wiki/Error-Code-TypesDoNotUnify for more information,
       or to contribute content related to this error.

I have also renamed withBoxBackTicks to markCodeBox

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 16, 2016

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 boxes library to support things spanning multiple lines. Is it really so bad to just leave things unquoted?

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 16, 2016

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

@texastoland
Copy link
Copy Markdown

texastoland commented May 16, 2016

I think it's #1517.

@hdgarrood
Copy link
Copy Markdown
Contributor

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?

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.02%) to 55.91% when pulling 9ab198a on soupi:0.9 into 4852b5d on purescript:0.9.

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 17, 2016

I can do that. should I do that?

@garyb
Copy link
Copy Markdown
Member

garyb commented May 17, 2016

In that case, it's probably just printName that needs to use markCode. Or any time a name appears in the middle of a sentence perhaps, so there are a bunch of other cases that are fine too.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 17, 2016

Yes please 😀

Sent from my iPhone

On May 16, 2016, at 9:57 PM, suppi notifications@github.com wrote:

I can do that. should I do that?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 20, 2016

how about putting ansi colors in the regular markCode code and when there's a flag to remove them, for example with --json-errors, filter them out?

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 20, 2016

We could, but I think it would be a bit cleaner to just not put them there in the first place.

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 27, 2016

screen shot 2016-05-27 at 3 18 38 pm

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.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.1%) to 56.061% when pulling 5adbd6f on soupi:0.9 into 83be9a5 on purescript:0.9.

purescript.cabal Outdated
utf8-string >= 1 && < 2,
vector -any
vector -any,
ansi-terminal >= 0.6.2 && < 0.7
Copy link
Copy Markdown
Contributor

@paf31 paf31 May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check if the output is a terminal?

@texastoland
Copy link
Copy Markdown

Purely aesthetic: make sure to test in terminal with a light theme. I recall yellow working at work though.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 27, 2016

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.

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 27, 2016

Worst case, we can provide a --monochrome flag.

@soupi
Copy link
Copy Markdown
Contributor Author

soupi commented May 27, 2016

Perhaps the Vivid color intensity is a bit too much for a light theme, maybe Dull will look better.
We can also support flags to change the color, or --monochrome, or choose a different color. I'm open to suggestions :)

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 55.994% when pulling 6956c35 on soupi:0.9 into 83be9a5 on purescript:0.9.

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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's pull the options out into a let binding here.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 55.994% when pulling 79d3460 on soupi:0.9 into 83be9a5 on purescript:0.9.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 55.994% when pulling 7624017 on soupi:0.9 into 83be9a5 on purescript:0.9.

@paf31 paf31 merged commit 2ca82f8 into purescript:0.9 May 28, 2016
@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 28, 2016

Thanks!

@texastoland
Copy link
Copy Markdown

What about just dimming the color?

@paf31
Copy link
Copy Markdown
Contributor

paf31 commented May 28, 2016

It should be dull yellow now.

archaeron pushed a commit to archaeron/purescript that referenced this pull request Apr 6, 2017
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants