Re: Column labels with --full

[Erik Auerswald]

Hey Tim,

On 04.06.22 04:58, Tim Rice wrote:
> > Maybe document it as deprecated, print a warning about the
> > combination, and turn it into a full blown error in a few releases?
> [...]
> After looking more closely at the issues, I've come to agree with Shawn 
> on this one. Rather than trying to invent a new meaning for something 
> that never really made sense, let's deprecate non-linewise uses of 
> -f/--full.
>
> If there were other problems like headers not having the right 
> operations printed, let's consider that a separate issue. For starters, 
> such a task would be a fair bit more difficult. Secondly, it will be 
> easier to reason about the issues when --full is not complicating the 
> matter.
>
> The attached patch makes an attempt at deprecating -f/--full for all 
> operations not in the MODE_PER_LINE category.

Thanks!

> The largest amount of work was to make sure the tests still make sense. 
> All the tests involving --full need to be assessed for whether they make 
> sense. The ones that don't require either rejigging to use a more 
> sensible operation, or else delete them.

I'd suggest to move nonsensical --full tests to a new test file,
e.g., tests/datamash-deprecated.…, instead of deleting them, to
both document and test the deprecation.  Or add a comment to the
current test file that this is deprecated and then add a check
for the deprecation message instead of changing the test inputs.

If and when the warning is turned into an error, those tests
could then be moved to a test file that checks for errors.  This
could be a new file tests/damash-errors.… or (some of) the tests
could be moved to an already existing tests/…-errors.… file.

Therefore I would suggest not to change existing --full tests in
order to make them pass without warning (without also preserving
the now failing test case adjusted to check for the warning).

If (some of) the new tests you introduce in the patch test some
option/command combination that is not yet tested, feel free to
add them.  This may well be in-place as done in the patch, but
then please consider keeping the old test in a new file to check
for the deprecation message.

> I'm not quite sure if I did this right.

You are adjusting some test inputs (e.g., exp_sort_in_header_full
in tests/datamash-sort-header.sh) in a way that seems unrelated
to the --full deprecation.  IMHO that makes it harder to review
the changes.

> Eg, when a test is deleted, 
> should all the tests after it be re-numbered to remain consecutive? Or 
> preserve the original numbering, so that all tests keep the same "uuid" 
> throughout history?

I'd suggest to renumber the tests to keep them consecutive, but to
do that in a second commit.  The first should IMHO comprise only
the actual changes without simple renumbering, the second should
only renumber.  That makes it much simpler to review the changes.

> Well, let me know what you think, or I'll merge it if I come back after 
> a day or two and it still looks okay to me.

I have only looked at the patch in your email, not tested it (yet).

Please prefix the warning with the program name.  That makes it
possible to see which program of a pipeline emitted a warning or
error message.  This is already done for error messages.

As I understand the translation part of the GNU project, wrapping
output in "_(…)" is used to allow adding translations of output.
You could consider wrapping the new warning output that way.

It seems as if GNU Datamash does not use an "error:" or "warning:"
prefix yet.  But I have the impression that using a lower case
"warning:" prefix would be more in line with the current code.
Or no such prefix at all (only the program name as prefix).

Since the new warning is just a fixed string, you could consider
using 'fputs (_("…"), stderr)' instead of 'fprintf (stderr, _("…"))'.

I'd like to suggest adding this change of behavior under a different
section header to the NEWS file.  The GNU Coreutils project uses
"** Changes in behavior", I'd suggest to use it as well.

I would like to ask you to consider prefixing the NEWS entry with
"datamash(1): ", since there are two programs in the GNU Datamash
project.

It seems to me as if "make syntax-check" might complain about too
long lines after applying the patch.

Cheers,
Erik