Adds support for comment lines in csv files #161

Closed
wants to merge 3 commits into
from

Conversation

6 participants

Hi,

this patch adds support for comment lines (i.e. lines that are going to be skipped during parsing) to CSV.
The standard behaviour of the class does not change. However, if the option :comment_marker => "#" is added to the constructor, all lines beginning with the specified marker are skipped.
Behaviour during CSV writing remains unchanged.
I've added tests for this option in test/csv/test_features.rb, to demonstrate usage and expected behaviour.

If this commit does not live up to some code or quality guidelines, I'd be happy to do my best to improve it.

By the way, thanks for the great work on ruby.

This pull request passes (merged 1f3b712 into f869ed2).

Member

zzak commented Aug 20, 2012

@JEG2 is the maintainer for csv, not sure if he maintains that as the faster_csv gem

JEG2 commented Aug 20, 2012

I'm fine with this change, but would prefer we generalize it. In other words, instead of :comment_marker, could we do something like skip_lines: /\A\s*#/? This might make it possible to skip to skip silly header lines with dates and such.

This was my first idea as well, but I didn't do it because I wasn't sure about the performance impact of matching an additional regex in each line. Or would that be negligible?

JEG2 commented Aug 20, 2012

I say who cares. :)

You only need to pay the price if you need that feature, right? And if you need it, you need it to get the parsing right.

That's my opinion. Thoughts?

Sounds great, I'll implement it and update the pull request accordingly.

This should do it. You can now pass any object that responds to matches as a :skip_lines parameter. If you pass any other object (except nil), an ArgumentError is thrown.
During parsing, any line matching skip_lines is skipped.

This pull request passes (merged 1578e5c into f869ed2).

JEG2 commented Aug 20, 2012

I am happy with this request and would like it merged. Can that be done from the Git side, or do I need to handle the changes in Subversion?

Member

drbrain commented Aug 20, 2012

@JEG2 you must commit through subversion. This repo is a read-only mirror.

Member

drbrain commented Aug 20, 2012

… including a "Fixes #161 on github" in your subversion commit message will automatically close this pull request.

@shyouhei shyouhei closed this in 3c3e659 Aug 20, 2012

Thank you for your feedback and the quick way to get this included.

JEG2 commented Aug 21, 2012

Thank you for doing the hard work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment