cdepillabout / pretty-simple

pretty-printer for Haskell data types that have a Show instance
https://hackage.haskell.org/package/pretty-simple
BSD 3-Clause "New" or "Revised" License
243 stars 29 forks source link

Highlight numbers #51

Closed ghost closed 5 years ago

ghost commented 5 years ago

This adds highlighting to numbers, closes #4. Also closes #49.

It's a pretty straightforward diff on the whole. We add a NumberLit constructor to Expr, and a OutputNumberLit to OutputType. These are hooked up in the obvious way.

Then we have a function parseNumberLit for parsing numbers. I've just handled integers (strings matching the regex [0-9]+) and decimals (strings matching the regex [0-9]+\.[0-9]+) for now. There is a question here about what exactly we want to treat as a number. You could imagine that maybe we would want to extend this in future to cover some or all of

A more tricky change is the modification of parseOther to accommodate this - as the syntax we care about has grown, parseOther needs to stop consuming input in more cases.

I chose to make it so that numbers as part of identifiers aren't highlighted as numbers. For example, given

data Foo = Foo { foo1 :: Integer , foo2 :: [String] } deriving Show
data Bar = Bar { bar1 :: Double , bar2 :: [Foo] } deriving Show

foo = Foo 3 ["hello", "goodbye"]

consider the result of pPrint foo

Foo 
    { foo1 = 3
    , foo2 = 
        [ "hello" 
        , "goodbye" 
        ] 
    } 

Do we want the '1' in foo1 and the '2' in foo2 to be highlighted as numbers? My feeling was that, as we are mainly (entirely?) showing Haskell data structures, it makes sense to leave foo1 and foo2 un-highlighted, whilst highlighting the bare 3 here. More generally, I implemented the rule that we don't highlight numbers that form part of Haskell-style identifiers.

cdepillabout commented 5 years ago

Thanks for implementing this! I'll try to take a look at it in the next few days.

ghost commented 5 years ago

No worries, whenever you can get to it of course. I saw the announcement about the recent release of v3, and (having enjoyed using this package before) thought that adding this feature would be nice and also relatively easy to implement. I hope the code looks reasonable to you.

cdepillabout commented 5 years ago

@lawrencebell I took a quick look at this. I think it looks good!

I did a small refactoring and added some tests in 3ea17eb. Can you do a quick review of this? Once you give the okay, I'll merge this in and cut a release to Hackage.

cdepillabout commented 5 years ago

@lawrencebell I went ahead and merged this in. If you take a look at this and notice anything strange, feel free to comment here, or open another PR fixing it.

Released on hackage as pretty-simple-3.1.0.0:

http://hackage.haskell.org/package/pretty-simple-3.1.0.0

ghost commented 5 years ago

@cdepillabout Apologies for the delay, I didn't find the time to look at this earlier. Looks great! I certainly wanted to add some tests, but wasn't sure how best to go about that. The doctests you added look like a nice addition.

I see in the refactoring there was one change to functionality, which is to make parseOther skip over upper-case identifiers like H3110 as well as lower case ones like hello234. Makes total sense - I realize now that I forgot to handle that case, and that we should handle it. It's nice to have a fresh pair of eyes spot things like that!

ghost commented 5 years ago

@cdepillabout Actually, I realize there's one other thing to consider here. We now have an opportunity to update the screenshots in the README to show off the number highlighting! That would be a nice thing to do, I think.

cdepillabout commented 5 years ago

We now have an opportunity to update the screenshots in the README to show off the number highlighting!

Yeah, I was about to do this when working on this PR, but I realized it required setting things like bash's PS1, and changing the prompt in GHCi as well, so I didn't do it.

However, if you wanted to send a PR doing this, I'd definitely merge it in. Also, it'd be really nice to include a script in the img/ directory that basically printed all the output that gets included in the screen shots. That would also make it easier to update in the future.

By the way, I released a blog post about updating pretty-simple:

https://functor.tokyo/blog/2019-08-15-pretty-simple-3.0.0.0

https://www.reddit.com/r/haskell/comments/cqc4f5/ann_prettysimple3000_a_simple_prettyprinting/

Probably not super interesting for you, but it seemed to go over well on reddit.