Skip to content

Highlight numbers#51

Merged
cdepillabout merged 8 commits intomasterfrom
unknown repository
Aug 19, 2019
Merged

Highlight numbers#51
cdepillabout merged 8 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Aug 14, 2019

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

  • hex/octal/binary syntax,
  • exponents in decimal notation (like 1e2 for example),
  • numeric underscores,
  • omitted leading digits (like .123),
  • leading plus/minus signs?

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.

lawrencebell added 4 commits August 14, 2019 22:53
Just handles integers (i.e. sequences of ASCII digits, "[0-9]+" as a
regex) for now, no decimals. So "12.34" gets interpreted as the integer
12 followed by a string "." followed by the integer 34.

Is also overly 'greedy', in that if we have
"Foo {foo1 = 'x', foo2 = 'y'}" say, the '1' in "foo1" gets treated as
an integer. As this is Haskell, we should probably respect "foo1" as an
identifier and leave the '1' alone.
As an example, consider the Haskell expression
"Foo {foo1 = 123, foo2 = 456}". Previously, we would treat the '1' in
"foo1" and the '2' in "foo2" as integers and highlight them
accordingly. With this commit, we instead consider strings like "foo1"
(that is, Haskell-style identifiers) as a plain string and leave the
'1' alone. (To be clear, we still highlight the 123 and 456 as integers
though.)
@cdepillabout
Copy link
Copy Markdown
Owner

cdepillabout commented Aug 15, 2019

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

@ghost
Copy link
Copy Markdown
Author

ghost commented Aug 15, 2019

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
Copy link
Copy Markdown
Owner

@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 cdepillabout merged commit 77a883a into cdepillabout:master Aug 19, 2019
@cdepillabout
Copy link
Copy Markdown
Owner

@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
Copy link
Copy Markdown
Author

ghost commented Aug 22, 2019

@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
Copy link
Copy Markdown
Author

ghost commented Aug 22, 2019

@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
Copy link
Copy Markdown
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

change doctests to use cabal-doctest highlight numbers in green

1 participant