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
experimental RFC4180 compliant (ish) quoting support #25
Closed
Closed
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
strip
ping 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 strippedText
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 otherReadable
instances, too. I think all we would need to do is have awhere part' = T.strip part
inf
, and then usepart'
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 amapAccumL
. It seems likefinish
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright I added the uncontroversial
strip
pending further discussion