Re: Patches for --vnlog support

[Dima Kogan]

Erik Auerswald <auerswal@unix-ag.uni-kl.de> writes:

>   b) A small nit: I am not really fond of using the short option
>      "-v" as alias for "--vnlog".  A "-v" option often is a short
>      form of "--verbose", sometimes short for "--version".
>      I would prefer to have no short option for "--vnlog" at all.
>      Since long options can be abbreviated, "--vn" would suffice
>      to activate "--vnlog" currently.

If we want a shorter version of the long option, let's do --vnl. This
makes it consistent with other things.


>   c) I would prefer if it you could move the contents of patch
>      0003-numerical-field-references-illegal-only-if-vnlog.patch
>      into the first patch.  That would ease understanding and
>      reviewing the patches.

The patch series developed over time. We can reshuffle things however
you like. Let's wait on more input first, though, so that we reshuffle
them just once.


>   d) The macro "IS_COMMENT" results in a semicolon (';') as
>      comment character inside vnlog data lines.  That is a
>      deviation from the vnlog description given at
>      <https://github.com/dkogan/vnlog/>.  I'd expect only
>      the Number Sign ('#') to start a comment with "--vnlog".

Yeah. I should fix that.


> 5) 0005-vnlog-added-to-help.patch
>
>   a) I think it would be more accurate to state that "--vnlog"
>      requires the use of the field name.  I'd suggest to add
>      a new sentence instead of just adding " or --vnlog".
>
>   b) I'd like it if you could add "--vnlog" information to
>      doc/datamash.texi as well.
>
>   c) I think it would be nice to add a bit more information
>      regarding the vnlog format to the man page (man/datamash.x)
>      and the texinfo documentation (doc/datamash.texi).
>
> 8) 0008-Tests-for-vnlog.patch
>
>   a) I'd prefer if you removed the commented out tests, I think
>      they distract from the actual contents.

This and the if(0) stuff you mention above are a form of documentation.


>   b) I'd prefer if you added a test that verifies that ';'
>      does not start an inline comment with "--vnlog".
>      Inline comments can be started with a semicolon with
>      the current patch set.
>
>   c) I'd prefer if you added tests that verify that ';'
>      cannot be used as a replacement for '#' with "--vnlog".
>      This does seems to work correctly with the current patch
>      set, but I would like to have tests for this difference
>      between existing GNU Datamash comment lines and "--vnlog"
>      comment and header lines.

OK.