experimental RFC4180 compliant (ish) quoting support #25
Conversation
tested a little and seems to work, haven't benchmarked or tried a wide variety of data
| finish :: ([T.Text], Maybe T.Text) -> [T.Text] | ||
| finish (rest, Just dangling) = dangling : rest -- FIXME? just assumes the close quote if it's missing | ||
| finish (rest, Nothing ) = rest | ||
|
|
There was a problem hiding this comment.
Preface to comments: this is a fantastic PR. I really appreciate how you've taken care to make useful, documented definitions.
Is there a problem here with not stripping the first pass at tokenization? Eg, if a line of input is "hey", "you, guys", the second comma-separated piece starts with a space. Of course, naively stripping those pieces is no good either as we need to preserve internal spaces, but the prefix/suffix predicates should probably be applied to stripped Text values. This makes using a space as a quote character impossible, but I think that's a pretty pathological corner case. We may also want to preserve the stripping behavior of the previous version to ease the life of other Readable instances, too. I think all we would need to do is have a where part' = T.strip part in f, and then use part' in the right places.
Another question is whether it makes sense to do this as a foldr rather than a a left-associated thing like a mapAccumL. It seems like finish is going to force the whole thing, so there's not an opportunity for incrementally computing the result.
This is also complicated enough that it deserves to have some test cases written for it. Once we're happy with this PR, we can hash out some basic tests.
There was a problem hiding this comment.
Re Preface: Thanks!
yes, I missed the strip step. RFC4180 specifies no spaces between the comma and the quote, but I could imagine that people might want different. I'd think that stripping non-quoted tokens and preserving spaces inside quotes is noncontroversial though. We could also have some kind of ParserOption to turn on stripping inside quotes?
I used foldr just to avoid reversing the list at the end in finish, no particular laziness intent.
and yeah, definitely has several corner cases and should have test cases to go with them
There was a problem hiding this comment.
alright I added the uncontroversial strip pending further discussion
|
Merged, thank you! |
tested a little and seems to work, haven't benchmarked or tried a wide variety of data
see issue #24