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

CsvFile abstraction #8

Merged
merged 2 commits into from
Oct 24, 2017
Merged

CsvFile abstraction #8

merged 2 commits into from
Oct 24, 2017

Conversation

CarsonF
Copy link
Contributor

@CarsonF CarsonF commented Oct 24, 2017

An adaption of what we've done in our other libraries/systems to read CSV files with the headers mapped to the rows values.
This one is based on SplFileObject which has a lot of the CSV logic builtin. Unfortunately, some of it is not handled very well so that's remedied here.
The actual iteration is more robust here as well, with better error handling and the ability to getHeaders and manually set them (in case the file doesn't have them).

This also works with CSV files with mac line endings \r using PHP's builtin auto detection. This throws an exception in our other implementations.

data associated with the file's headers as the keys.
@CarsonF
Copy link
Contributor Author

CarsonF commented Oct 24, 2017

@mikegillis677 Can you look over those additions for writing? I wanted to implement some kind of "auto header" writing but couldn't figure out a good way to do it. Plus it was already complex with headers being able to be read from file, keys from first row written, or set manually.

I think what I have here covers all of our use cases. I don't think redshift CSVs have headers, but maybe I can't remember correctly. Those are the only ones we write to. The geo ones are only read from so they should already be good from the first commit.

@mikegillis677
Copy link
Contributor

The changes look sane. I checked, and Redshift CSVs do not have headers.

@CarsonF CarsonF merged commit a7ce996 into master Oct 24, 2017
@CarsonF CarsonF deleted the feature/csv branch October 24, 2017 21:44
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