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

Support for comments in CSV input. #51

Merged
merged 1 commit into from
Jul 15, 2014
Merged

Support for comments in CSV input. #51

merged 1 commit into from
Jul 15, 2014

Conversation

runejuhl
Copy link
Contributor

I'm not sure if there's a basis for this functionality in PapaParse, but I stratched my own itch and you might as well benefit :)

It is configurable by setting config.commentChar to either a string, true (in which case it defaults to using "#") or false.

…mentChar to either a string, true (in which case it defaults to using "#") or false.
@runejuhl
Copy link
Contributor Author

I thought I had tested well, but there seems to be a bug when header: false. I'll close it for now and have a look at it tomorrow. Sorry for the noise.

@runejuhl runejuhl closed this Jun 11, 2014
@mholt
Copy link
Owner

mholt commented Jun 11, 2014

Looking forward to the revised PR!

@runejuhl
Copy link
Contributor Author

So, had a look at it. I can't seem to figure out what was wrong yesterday, but it might've had something to do with Firebug 2.0/Firefox 30 breaking Sencha ExtJS. I'm unable to reproduce any errors with Firebug Scripts pane disabled in Firefox 31 and in stock Chromium 35.
If you have some more extensive tests, feel free to throw them at me, I'd be glad to run them.

@runejuhl runejuhl reopened this Jun 12, 2014
@mholt
Copy link
Owner

mholt commented Jun 14, 2014

First off, thanks for contributing!

It looks like the existing tests still pass, which is good, but when you have a chance, would you write test(s) to assert the behavior of the new comment feature? For instance, setting the comment character to the delimiter causes an infinite loop in the tests page. I know that's an edge case, but it's just another way to be defensive that we should consider.

As an aside, I just implemented this into version 3 of Papa Parse, complete with a whole new set of tests, including those for commented lines.

I'll be happy to merge this in with appropriate tests (that's just policy when changing the Parser).

@mholt
Copy link
Owner

mholt commented Jul 15, 2014

I'm about ready to deploy version 3.0. As such, I'm going to merge this in so you get credit on the contributors page since you made the effort, and the feature persists in this next version. Thanks again!

mholt added a commit that referenced this pull request Jul 15, 2014
Support for comments in CSV input.
@mholt mholt merged commit 7ad465e into mholt:master Jul 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants