[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Patches for --vnlog support

From: Dima Kogan
Subject: Re: Patches for --vnlog support
Date: Sun, 07 Aug 2022 14:15:12 -0700
User-agent: mu4e 1.8.6; emacs 29.0.50

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]