Skip to content

Feature/fix line ending #7

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

Merged
merged 3 commits into from
Aug 15, 2016
Merged

Conversation

phoney
Copy link
Contributor

@phoney phoney commented Aug 3, 2016

This pull request adds code to optionally sniff line endings of CSV files. The user can specify the line ending if they know it or let CSVImporter determine it automatically. Some test were added.

phoney added 3 commits August 3, 2016 15:09
Calling code can optionally specify the line ending. If they don't specify the line ending then it will be sniffed automatically. By passing the correct line ending to the streamReader() no line ending characters are included in the lines returned from the streamReader. Note that any files with mixed line endings will result in undefined behavior. But it was always like that.
If the correct lineEnding is set for the streamReader no line ending characters will be returned as part of lines. So there's no need to check for them in the line contents.
@Jeehut
Copy link
Member

Jeehut commented Aug 4, 2016

Looks good at a first glance, thanks! 👍

Will have a detailed look and merge soon.

@phoney
Copy link
Contributor Author

phoney commented Aug 4, 2016

OK, sounds great.

One comment: I made the lineEndingForFile() method private. It could be public.

@Jeehut
Copy link
Member

Jeehut commented Aug 15, 2016

I just had a detailed look and except for some code style issues (mostly wrongly set whitespaces) it is good. Merging it and tackling the code style changes later on. Good work! 👍

@Jeehut Jeehut merged commit fd9af0f into FlineDev:stable Aug 15, 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