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

Remove nil values from rows #4

Closed
wants to merge 1 commit into from
Closed

Conversation

pezholio
Copy link
Contributor

@pezholio pezholio commented Jan 7, 2014

It seems that the CSV parser returns nil if there is a comma and then a blank. I don't think we want this, so I've stripped nil values from the generated array. Now https://www.gov.uk/government/uploads/system/uploads/attachment_data/file/246500/New_Year_Honours_2013_full_list.csv/preview returns loads of errors, as expected

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling cf65d28 on bugfix-remove-nil-from-rows into ba034f4 on master.

@Floppy
Copy link
Member

Floppy commented Jan 7, 2014

Will it return the same number of nils even if there are commas missing? Because I'd expect an empty field to come back nil, so this seems... dangerous.

@JeniT
Copy link

JeniT commented Jan 7, 2014

Are you sure that's the right behaviour? It's OK for there to be empty cells within a row. Does this make the distinction between missing values within cells and missing cells, ie between

a,b,c
x

which should give errors and:

a,b,c
x,,

which shouldn't?

@Floppy
Copy link
Member

Floppy commented Jan 7, 2014

Yeah, that ^

@pezholio
Copy link
Contributor Author

pezholio commented Jan 7, 2014

Yeah, that's why I wasn't sure. I wasn't getting any errors on

a,b,
a,b,c

Which actually should be right. So ignore me.

@pezholio pezholio closed this Jan 7, 2014
@pezholio pezholio deleted the bugfix-remove-nil-from-rows branch January 7, 2014 20:24
pezholio pushed a commit that referenced this pull request Jul 24, 2015
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.

4 participants