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

FIX #45 Option to skip ending line break (csv) #46

Closed
wants to merge 1 commit into from

Conversation

mathieu-lavigne
Copy link

@mathieu-lavigne mathieu-lavigne commented Oct 23, 2017

Fixes #45.

@cowtowncoder
Copy link
Member

Ok. Thank you for submitting this! It'd be great if logic would prevent outputting instead of trying to undo addition as undo seems bit error-prone. I'll see if that is doable.

@mathieu-lavigne
Copy link
Author

You're totally right @cowtowncoder. I tried that way first but it seemed to me to be more confusing. One reason is that you have to know when you reach the last line or not.

@cowtowncoder cowtowncoder changed the title FIX #45 Option to skip ending line break FIX #45 Option to skip ending line break (csv) Nov 30, 2017
@OmarHawk
Copy link

wow, this is open for a very long time.... I actually do need this...

@cowtowncoder
Copy link
Member

It'd be good to resolve the conflict, could consider merging.

@cowtowncoder
Copy link
Member

@mathieu-lavigne Apologies for dropping the ball on this. I think this would work, as CsvEncoder.endRow() does guarantee that linefeed is not split on output buffer boundary. There may be some edge cases still (someone calling CsvGenerator.flush() at the end before close()...) but that is probably acceptable considering how difficult alternative is (that of trying to change how linefeed is added).

But I'd like one bigger change: instead of CsvSchema, I think this really works better as CsvGenerator.Feature.
There's also need for a unit test or two.

With those I'd be happy to merge this for 2.17.

I realize you may not have time or interest after this delay; if not, I understand. And can then proceed with a new PR.

@cowtowncoder
Copy link
Member

Created new PR #453 to implement as generator feature.

Found couple of issues wrt implementation -- generator.flush() will prevent it from working, basically -- but aside from that works ok (added couple of basic tests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants