Re: Column labels with --full

[Tim Rice]

Hey Erik,

Thanks for the feedback. (Sorry for the delayed reply. Saturdays are when I can 
get the most done on GNU Datamash. The rest of the week can be a bit iffy.)


> 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.

Yep, makes sense. I'll adjust my changes to keep this in mind.


> 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.

Well, those changes are for the output, not the input, because 
datamash-sort-header.sh is set up differently to the other tests.

The input is always from here:

```
# An unsorted input with a header line
INFILE="x y z
A % 1
B ( 2
A & 3
B = 4"
```

Most (all?) of the tests in that file follow the pattern of `echo $INFILE | 
datamash ...`.

When the datamash operation changes, the output changes, which is what the 
patch addresses.

Perhaps we should adjust this test to be more like the perl-based ones, but 
that sounds like a job for another patch :)


> 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.

No worries, that's easy.


> 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, _("…"))'.

Good tips, I'll keep this in mind.


> 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.

Ah yeah, I should use `make syntax-check`, I forgot about that. I'll make the 
other adjustments too.

Thanks again!

~ Tim