escape strings to satisfy Dot syntax rules #47
Conversation
|
Things I know need doing (at least)
|
|
Simple test program |
src/Algebra/Graph/Class.hs
Outdated
| class Escape s where | ||
| escape :: s -> s | ||
| instance Escape String where | ||
| escape = concatMap (\x-> if x == '"' then "\\\"" else [x]) |
There was a problem hiding this comment.
Since this is DOT-specific escaping, I suggest to move this class to Export.Dot.
src/Algebra/Graph/Export/Dot.hs
Outdated
| defaultStyleViaShow :: (Show a, IsString s, Monoid s) => Style a s | ||
| defaultStyleViaShow = defaultStyle (fromString . show) | ||
| defaultStyleViaShow :: (Show a, IsString s, Escape s, Monoid s) => Style a s | ||
| defaultStyleViaShow = defaultStyle (fromString . escape . show) |
There was a problem hiding this comment.
Ahh, I just realised that since we can escape after converting to String we don't need the constraint, and we might not even need the typeclass!
There was a problem hiding this comment.
I've just realised this too... at least in the context of defaultStyleViaShow. I don't know whether you'd agree Alga should sanitize some of the other paths e.g. exportAsIs and whether we'd need the typeclass for that.
|
@jmtd Thank you! I just realised that I might have overcomplicated things by suggesting to add the type class, since we can escape after we have the As for generic |
I suppose the question is, since Alga.Graph.Dot generates the output for graphviz, do you want to promise it will output something valid, no matter construction the user has thrown at you, and whether that is feasible. |
|
@jmtd What if the user has already provided escaped strings, e.g. we have Note that in |
GraphViz's "dot" language has several options for encoding IDs, as described here: https://www.graphviz.org/doc/info/lang.html Alga makes use of the quoted-string syntax. This permits any character within double quotes, except the double-quote character itself, which must be prefixed by a backslash character. (the backslash itself does not need to be escaped). Implement the escaping of " for defaultStyleViaShow.
998d3a3 to
ab89e0c
Compare
|
I've just pushed a simplified patch that only implements escape for defaultStyleViaShow. I've gone for a simpler escape implementation than earlier which I think performs much better and is debatably simpler to understand too. I still need to write tests and docs. |
|
@jmtd Thanks! I have a suspicion that this might be too simple and fail to handle this case correctly: -- A string containing a single character "
test1 :: String
test1 = "\""Will the current implementation turn it into Perhaps, we need to escape the character We might also add a test specifically for -- A string containing a single character \
test2 :: String
test2 = "\\" |
|
Yes, the current implementation will generate that, and dot will not accept it: Relatedly, a strict reading of the Dot language description would lead one to believe (all these backslashes! even GH's markdown parser is having trouble :)) My next aim is to write some tests to capture this, within your existing test framework (which I'm interested to learn more about; I've only used HTF for my own things before, but it looks to me that HTF is not in wide use any more) |
|
My 'test framework' is not something I'm proud of and could definitely be improved (#13). However, for this PR it should be fairly straightforward to add tests somewhere around here: https://github.com/snowleopard/alga/blob/master/test/Algebra/Graph/Test/Export.hs#L151-L162 I like to keep docs and tests in sync, so that all invariants are documented and are not surprising to users. |
|
By the way, it may be useful to expose the escaping functionality for two reasons:
The cost is that we'll need to maintain it, but it doesn't seem to high. |
GraphViz's "dot" language has several options for encoding IDs, as
described here: https://www.graphviz.org/doc/info/lang.html
Alga makes use of the quoted-string syntax. This permits any character
within double quotes, except the double-quote character itself, which
must be prefixed by a backslash character. (the backslash itself does
not need to be escaped).
In order to implement this requirement, introduce an "Escape" typeclass
and "escape" function.
Provide an instance definition for regular Haskell Strings. This
requires the FlexibleInstances Syntax extension.