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

Fix string escaping #19

Merged
merged 1 commit into from
Jan 25, 2018
Merged

Fix string escaping #19

merged 1 commit into from
Jan 25, 2018

Conversation

Merovius
Copy link
Contributor

If a string contains quote-characters, they don't get correctly
escaped, causing syntax errors in the resulting graphviz file.
This quotes the string using strconv and then removes the
quote-characters added by that.

@bradleyjkemp
Copy link
Owner

Fails because tests currently expect strings to be quoted. I think you just need to append and prepend "\"" to the string you've got though and then this should be good to go. Thanks :)

@Merovius
Copy link
Contributor Author

Merovius commented Jan 25, 2018

Hm, I think I should have a 2nd look, though. If I do that, the tests pass, but the svg-output looks like this which seems suboptimal. I updated the PR. The tests pass (at least locally) and the output looks like a go string literal, which is probably what we want.
But I do feel a tiny bit dirty about the resulting code…

@bradleyjkemp
Copy link
Owner

Hah yeah the double strconv.Quote is a bit surprising. Can you just add a quick comment to explain this (i.e. that the first Quote makes the string look like a go literal and then the second one escapes it for graphviz) and then I'll merge?

If a string contains quote-characters, they don't get correctly
escaped, causing syntax errors in the resulting graphviz file.
This quotes the string *twice*: The first quoting makes it a go-quoted
string literal - the second quoting gets unescaped by graphviz.
We then remove the outermost quotes.

As a result, the string appears as a go-quoted string literal in the
output, which seems to be, what we want.
@Merovius
Copy link
Contributor Author

Done.

@bradleyjkemp bradleyjkemp merged commit b236a53 into bradleyjkemp:master Jan 25, 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants