search

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

Quick fix for issue #40 #41

Closed anka-213 closed 5 years ago

anka-213 commented 5 years ago

I simply added a case for ignoring the next character after a backslash in parseStringLit.

Fixes #40.

anka-213 commented 5 years ago

An alternative would be to replace the parseStringLit function with reads @String and remove the readMaybe from render, but that wouldn't handle malfromed and unclosed strings as nicely without further work.

Btw, the old version of parseStringLit could be replaced with parseStringLit = first (drop 1) . span (/='"'). Same thing with parseOther which can be replaced with

parseOther :: String -> (String, String)
parseOther = span (flip notElem "{[()]}\",")
anka-213 commented 5 years ago

I should also add a test case for this.

andrew-lei commented 5 years ago

Thanks, good catch! I will write up a test case and merge.

The reason parseStringLit is at it is currently is because I had hoped this could be made entirely lazy. But as you can see, that's currently not the case as render will not lazily render a string literal. This is because there is the possibility of invalid escape characters appearing in a string literal input, and how should that render as. We didn't think there was an objective answer to that, so I had hoped to modify this to allow for some configurability in that respect (in which case, the string literal rendering in render would be just one possible way to render), but unfortunately I hadn't gotten around to doing that.

cdepillabout commented 5 years ago

@anka-213 Yes, a test case would be great!

@andrew-lei After @anka-213 adds a test case, does this look reasonable to merge? If so, please feel free to go ahead and merge it.

andrew-lei commented 5 years ago

Yes, in fact I've already added the test from #40. Although I don't think I did the commit correctly since it ended up on a different branch instead of to this pull request. Regardless, when Travis is done checking it, I will merge that branch.

cdepillabout commented 5 years ago

Okay thanks @andrew-lei!

andrew-lei commented 5 years ago

OK merged.

anka-213 commented 5 years ago

@andrew-lei Regarding the lazy parsing, we might be able to use the function lexChar from the module Text.Read.Lex to lazily read one character at a time and avoid the need to parse the entire string at once. I'll see if I can write a working lazy version with it.