Avoid to generate text with ending backslash #225
Conversation
There was a problem hiding this comment.
@gabrielelana Thanks for your contribution! I appreciate improvements to tests 🙂
We were wondering why you didn't use genPrefixMap in genToml
Actually, this seems like a unfortunate thing. genPrefixMap should be used to generate TOML. Could, you, please, try to use genPrefixMap instead of the current solution?
test/Test/Toml/Gen.hs
Outdated
| genText :: MonadGen m => m Text | ||
| genText = fmap Text.concat $ Gen.list (Range.constant 0 256) $ Gen.choice | ||
| genText :: (MonadGen m, GenBase m ~ Identity) => m Text | ||
| genText = Gen.filter invalidEscape $ fmap Text.concat $ Gen.list (Range.constant 0 256) $ Gen.choice |
There was a problem hiding this comment.
Issue #198 is about not generating \\ inside TOML keys. However, I believe that genText is used not only for keys but for something else. So filtering here is not exactly correct. Instead, you should filter only when generating TOML keys.
There was a problem hiding this comment.
I think genText is not used to generate keys but I also thought that a generic text should not end with a single escape like "aaa\"
test/Test/Toml/Gen.hs
Outdated
| invalidEscape :: Text -> Bool | ||
| invalidEscape t | ||
| | t == Text.empty = True | ||
| | Text.last t == '\\' = False | ||
| | otherwise = True |
There was a problem hiding this comment.
Can you please some documentation to this function and some examples of valid and invalid strings? And ideally with the link to TOML spec where it says that "\" cannot be a valid key. I'm afraid that this this solution solves the problem partially. Is the following key valid?
"\x"I think that no, but it still can be genarated. I'm okay to accept this PR as a partial solution, but let's document some cases of what this function covers and what it doesn't cover 🙂
There was a problem hiding this comment.
Look at validText in the last commit if it's what you asked
test/Test/Toml/Gen.hs
Outdated
| validString ('\\':'"':s) = validString s | ||
| validString ('\\':'\'':t) = validString t | ||
| validString ('\\':'\\':t) = validString t | ||
| validString ('\\':'u':n1:n2:n3:n4:t) = (all Char.isHexDigit [n1, n2, n3, n4]) && validString t |
There was a problem hiding this comment.
This function is becoming too complicated... I'm afraid it will require some time to understand what's happening here and make sure that there's no better approach... At this point the previous solution doesn't look that bad 🙂
There was a problem hiding this comment.
Bummer, I thought it was pretty slick 😂 Anyway, no problem to revert it
|
@chshersh let me know what do you think |
There was a problem hiding this comment.
@gabrielelana This should be good for now 👍 Thanks for your contribution! The generator can be patched again if we ever have troubles with it 🙂
Fixes half of #198
We were wondering why you didn't use
genPrefixMapingenToml, seems like the second problem stated in #198 it might be related togenPrefixMap, are we on the right path?Proudly made by @sphaso and @gabrielelana at "Open Source Saturday Milano"