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

Add CsvEncoder #486

Merged
merged 7 commits into from
May 22, 2023
Merged

Add CsvEncoder #486

merged 7 commits into from
May 22, 2023

Conversation

dr0i
Copy link
Member

@dr0i dr0i commented May 19, 2023

See #483 .

Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should call out metafacture-csv-plugin as the "inspiration" (to put it mildly). And also deprecate the plugin as a consequence of being merged into core.

@blackwinter blackwinter assigned dr0i and unassigned blackwinter May 19, 2023
@dr0i
Copy link
Member Author

dr0i commented May 19, 2023

Maybe we should call out metafacture-csv-plugin as the "inspiration" (to put it mildly).

Right. It's a copy. I thought to add @eberhardtj as author but I thought the author is hijacked resp. not the original one, because it's somehow ghosted (https://github.com/metafacture/metafacture-csv-plugin) and her github history starts 2019 and it's spares. So I mentioned at least the dnb as first contributor.
Shall I force-push , mentioning the original commiter's email address like the one in 087e20b ?

And also deprecate the plugin as a consequence of being merged into core.

+1 doing this when merged.

@blackwinter
Copy link
Member

but I thought the author is hijacked resp. not the original one, because it's somehow ghosted (https://github.com/metafacture/metafacture-csv-plugin) and her github history starts 2019

Yes, I would probably just use the user name without the @, so no direct connection to the GitHub user is created. But e-mail address is fine as well (or even better) if you got one.

Shall I force-push , mentioning the original commiter's email address like the one in 087e20b ?

Yes, sounds good. If it were me I would also reference the original repo, so one can follow the Git history.

dr0i and others added 5 commits May 20, 2023 15:26
@dr0i
Copy link
Member Author

dr0i commented May 20, 2023

Reworked PR. Please have a look again @blackwinter .

@dr0i dr0i assigned blackwinter and unassigned dr0i May 20, 2023
Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the unit test from my previous comment in f3b3d92.

You could replace the setup() method with a MockitoRule, but that's up to you.

@TobiasNx
Copy link
Contributor

Where is the documentation in https://github.com/metafacture/metafacture-documentation/blob/master/flux-commands.md
or how is this updated? Since @fsteeg said this is automatically created?

@dr0i
Copy link
Member Author

dr0i commented May 22, 2023

The flux-commands.md is not yet automatically updated AFAIK (there is a ticket somewhere ...). I do this regularly by hand at least when doing a release and at best after merging the PR.

@dr0i
Copy link
Member Author

dr0i commented May 22, 2023

I've added the unit test from my #486 (comment) in f3b3d92.

ah - sorry! Amending the commits and force pushing must have lost these. Good that you checked that :)

Based on a suggestion by Jens Wille.
@TobiasNx
Copy link
Contributor

I tested without options and | encode-csv(includeHeader="TRUE", separator="\t", noQuotes="true") seem to work nicely!

@dr0i dr0i merged commit 944e819 into master May 22, 2023
1 check passed
blackwinter added a commit that referenced this pull request May 22, 2023
@dr0i dr0i deleted the 483-addCsvEncoder branch May 22, 2023 11:34
dr0i added a commit to metafacture/metafacture-documentation that referenced this pull request May 22, 2023
@dr0i dr0i mentioned this pull request May 22, 2023
@dr0i
Copy link
Member Author

dr0i commented May 30, 2023

And also deprecate the plugin as a consequence of being merged into core.

Done this, see https://github.com/metafacture/metafacture-csv-plugin/blob/master/README.adoc.

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

Successfully merging this pull request may close these issues.

None yet

3 participants