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

Fixes for CSV Header Conversion #575

Closed
wants to merge 2 commits into from
Closed

Fixes for CSV Header Conversion #575

wants to merge 2 commits into from

Conversation

sshaw
Copy link

@sshaw sshaw commented Mar 21, 2014

1st commit: don't try to encode nil headers. Currently this causes:

NoMethodError: undefined method `encode' for nil:NilClass

2nd commit: just makes sense for this converter, I think. Currently leading/trailing whitespace will create: :_symbol_header or :symbol_header_.

@zzak
Copy link
Member

zzak commented Mar 31, 2014

cc @JEG2

@JEG2
Copy link

JEG2 commented Mar 31, 2014

I think adding the strip() call is pretty safe. It does technically change the results you would get with certain files, but I think it's probably for the better.

I'm less sure swallowing the nil headers. If we do that, it rules out the possibility of writing a converter that changes nil headers into what you would like them to be.

Thoughts?

@sshaw
Copy link
Author

sshaw commented Apr 1, 2014

I'm less sure swallowing the nil headers.
... it rules out the possibility of writing a converter that changes nil headers into what you would like them to be.

True, but what's the use case? Not much context is provided for nil headers. One could append a counter to the generated name (how would one then make the relation between the auto generated name and its values) or use a constant value as a catch-all.

That aside, the current code (NoMethodError aside) doesn't necessarily accommodate this. One would either have to put their converter before the builtins or the builtins would have to convert nil to a String to prevent header processing from being halted.

@evanphx evanphx closed this in 1170b05 Apr 1, 2014
@JEG2
Copy link

JEG2 commented Apr 1, 2014

You convinced me. I applied both of your changes.

Thank you.

mmasaki pushed a commit to mmasaki/ruby that referenced this pull request Apr 21, 2014
  Reported by Skye Shaw
  [Fixes rubyGH-575]



git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@45498 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
hsbt pushed a commit that referenced this pull request Jan 23, 2016
* lib/csv.rb: Update documentation of CSV header converter for
  r45498, [GH-575].  [Fix GH-1215]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@53628 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
mrkn pushed a commit to mrkn/ruby that referenced this pull request Apr 17, 2016
* lib/csv.rb: Update documentation of CSV header converter for
  r45498, [rubyGH-575].  [Fix rubyGH-1215]

git-svn-id: svn+ssh://svn.ruby-lang.org/ruby/trunk@53628 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
matzbot pushed a commit that referenced this pull request Jun 27, 2023
(ruby/irb#615)

This makes sure `Context#evaluate` really just evaluates the input.
It will also make #575's implementation cleaner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants