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

feat!: use CSVWriter interface instead of csv.Writer in SafeCSVWriter #233

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gfaugere
Copy link

@gfaugere gfaugere commented Jan 2, 2023

Using the interface instead of the encoding/csv.Writer allows users to use truly custom writers, or to add their own wrappers in the writer chain.

We have a use case where we want to transform our fields before writing, and allowing us to wrap the writer while keeping the lib provided SafeCSVWriter would be quite cool!

This is a breaking change as it changes the contract of type SafeCSVWriter (props from the csv.Writer can no longer be accessed from it), but I think it would be better to decouple these two?
Let me know if you think there is any downside to this? I don't see any but I am quite new with Go

BREAKING CHANGE: it is no longer possible to access and modify
underlying properties (such as Comma) of csv.Writer from
a SafeCSVWriter instance.
@pikanezi
Copy link
Member

Hi, as I don't use this library I'm not sure if this breaking change will impact a lot of people or not, I would like some feedback from other users if you don't mind 🙂

@ess
Copy link

ess commented Jun 20, 2024

Howdy @gfaugere !

I was just about to create a fork to do exactly this. While you may have been relatively new to Go in January of last year, I'll go as far as to say that the only thing that would hold me back from merging this (if I had that capability) would be changes to the documentation and examples.

From a usability standpoint, I don't even see this as a breaking change, really. In my mind, it seems like modifying the delimiter that your CSV writer uses in-situ is a pretty bad idea from the jump, so this would truly make it a Safe CSVWriter 😁

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.

3 participants