2018-03-08 11:05 GMT+01:00 Fabien COELHO <coe...@cri.ensmp.fr>: > > 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. >
In this case recordsep is not problem - default is ok. > > 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.
