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

[#1068] Add delimiter option to parser_csv #1108

Merged
merged 4 commits into from
Jul 19, 2016

Conversation

llby
Copy link
Contributor

@llby llby commented Jul 17, 2016

  • some test dosen't run. because it has not arg 'param'.
  • CSV.parse_line return nil from empty. so flag 'null_empty_string' do nothing.
  • and same problem has test_parser_tsv.rb

@tagomoris
Copy link
Member

Do you mean that adding col_sep argument to CSV.parse_line changes default behavior for empty string?
I think it's not acceptable for many users. Are there any way to help it?

@llby
Copy link
Contributor Author

llby commented Jul 19, 2016

col_sep is not affected.
CSV.parse_line change ,, to nil,nil,nil from the beginning.
test_parse_with_null_value_pattern is not work.
because lost (param).
so I modify to test pass.

@tagomoris
Copy link
Member

Okay, I understood that test-unit has a bug for such case: test-unit/test-unit#122

def parse(text)
yield values_map(CSV.parse_line(text))
yield values_map(CSV.parse_line(text,{col_sep: @delimiter}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use keyword argument style instead of specifying a hash object: CSV.parse_line(text, col_sep: @delimiter)

@tagomoris
Copy link
Member

I created a pull-request to fix bugs you pointed: #1112
Could you rebase your change on master after merging it?

@tagomoris
Copy link
Member

I merged #1112 .

@llby
Copy link
Contributor Author

llby commented Jul 19, 2016

done.

@tagomoris
Copy link
Member

Great addition. Thanks!

@tagomoris tagomoris merged commit 732aae8 into fluent:master Jul 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants