metafacture / metafacture-core

Core package of the Metafacture tool suite for metadata processing.
https://metafacture.org
Apache License 2.0
69 stars 34 forks source link

Add CsvEncoder #486

Closed dr0i closed 1 year ago

dr0i commented 1 year ago

See #483 .

dr0i commented 1 year ago

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 087e20b428893b396d6a87a97ce5d6f99a38c51a ?

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

+1 doing this when merged.

blackwinter commented 1 year ago

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 commented 1 year ago

Reworked PR. Please have a look again @blackwinter .

TobiasNx commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I've added the unit test from my https://github.com/metafacture/metafacture-core/pull/486#discussion_r1199086080 in https://github.com/metafacture/metafacture-core/commit/f3b3d92e162b45cc923daf169d41a0db371d8384.

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

TobiasNx commented 1 year ago

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

dr0i commented 1 year ago

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.