Hello Daniel,

PFA a v3 patch that implements that, along with regression tests this time.

My 0.02 €:

Patch applies cleanly, compiles, make check ok, doc generation ok.

I'm in favor of having a simple psql way to generate a convenient and compliant CSV output for export/import.

I also think that a short option brings little value, and "--csv" is good enough for the purpose, so I would agree to remove the "-C" binding.

About "fieldsep_csv", I do not like much the principle of having different output variables to represent the same concept depending on the format. I would rather have reused fieldsep as in your previous submission and set it to "," when under --csv. This is not a strong opinion and other people may differ: the committer opinion is the one to follow:-)

The "\n" eol style is hardcoded. Should it use "recordsep"? For instance, https://tools.ietf.org/html/rfc4180 seems to specify CRLF end of lines. The definition is evolving, eg https://www.w3.org/TR/tabular-data-model/ accepts both "\r" and "\r\n". I do not like using windows eol, but I think that it should be possible to do it, which is not the case with this version.

The "\pset format" error message in "do_pset" shows values in seemingly random order. The situation is pre-existing but not really satisfactory. I'd suggest to put all values in alphabetical order.

In csv_print_field & csv_print_text, you are not consistent when putting braces on blocks with only one instruction. I'd suggest not to put braces in that case.

I'd suggest that tests should include more types, not just strings. I would suggest int, float, timestamp, bytea, an array (which uses , as a separator), json (which uses both " and ,)...

--
Fabien.

Reply via email to