Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quick fix for issue #40 #41

Merged
merged 1 commit into from Nov 21, 2018
Merged

Quick fix for issue #40 #41

merged 1 commit into from Nov 21, 2018

Conversation

@anka-213
Copy link
Contributor

@anka-213 anka-213 commented Nov 20, 2018

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

Fixes #40.

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.

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
Copy link
Contributor Author

@anka-213 anka-213 commented Nov 20, 2018

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
Copy link
Contributor Author

@anka-213 anka-213 commented Nov 20, 2018

I should also add a test case for this.

@andrew-lei
Copy link
Collaborator

@andrew-lei andrew-lei commented Nov 20, 2018

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

@cdepillabout cdepillabout commented Nov 20, 2018

@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
Copy link
Collaborator

@andrew-lei andrew-lei commented Nov 20, 2018

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

@cdepillabout cdepillabout commented Nov 21, 2018

Okay thanks @andrew-lei!

@andrew-lei andrew-lei merged commit f8a545d into cdepillabout:master Nov 21, 2018
1 check passed
andrew-lei added a commit that referenced this issue Nov 21, 2018
@andrew-lei
Copy link
Collaborator

@andrew-lei andrew-lei commented Nov 21, 2018

OK merged.

@anka-213
Copy link
Contributor Author

@anka-213 anka-213 commented Nov 21, 2018

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

@anka-213 anka-213 deleted the patch-1 branch Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants