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

If skip_lines is set to a String, convert it to a Regexp #455

Closed
wants to merge 1 commit into from

Conversation

@kstevens715
Copy link

@kstevens715 kstevens715 commented Nov 23, 2013

This pull request fixes the issue I described in: https://bugs.ruby-lang.org/issues/8560

Basically, skip_lines takes any object that responds to match and passes in each line of the csv as an argument to match. But when skip_lines is a string, string#match converts it's argument to a Regexp. In our case, that argument is each line in the CSV. So each line gets converted to a Regexp one at a time.

It is very unexpected behavior for each line in the CSV to act as a Regexp. And in many cases, it will cause an error similar to:

2.0.0-p247 :001 > "test".match "#("
RegexpError: end pattern with unmatched parenthesis: /#(/
    from (irb):1:in `match'
    from (irb):1
    from /home/kyle/.rvm/rubies/ruby-2.0.0-p247/bin/irb:12:in `<main>'

Forced to make the choice between converting skip_lines to a Regexp if it's a String and converting each line in the CSV to a Regexp, I've chosen the former.

@kstevens715
Copy link
Author

@kstevens715 kstevens715 commented Nov 23, 2013

/cc @JEG2

@JEG2
Copy link

@JEG2 JEG2 commented Nov 23, 2013

This patch is perfect and ready to apply. Can we merge it here or does it need to be done via Subversion?

@drbrain
Copy link
Member

@drbrain drbrain commented Nov 23, 2013

It needs to be committed via subversion.

@nobu
Copy link
Member

@nobu nobu commented Nov 23, 2013

The expected value should come first to assert_equal.

to prevent the alternative, which is that each line in the CSV gets
converted to a Regexp when calling skip_lines#match.
@kstevens715
Copy link
Author

@kstevens715 kstevens715 commented Nov 23, 2013

Thanks guys. I've fixed the assertions to have expectations come first and I've generated a patch that I've attached to the original issue in Redmine.

@JEG2
Copy link

@JEG2 JEG2 commented Nov 23, 2013

I've applies the patch. We can close this pull request.

@drbrain drbrain closed this Nov 23, 2013
@drbrain
Copy link
Member

@drbrain drbrain commented Nov 23, 2013

@JEG2 in the future you can close pull requests by including a comment like this the commit message:

f67be32

@JEG2
Copy link

@JEG2 JEG2 commented Nov 23, 2013

Roger. Thanks.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.