Re: Column labels with --full

[Shawn Wagner]

About make syntax-check

I keep forgetting to run it too and have been thinking about adding a pre-commit hook that does it automatically on my local repo.

(Too bad it's a client side hook that isn't included in git clone.)

On Mon, Jun 6, 2022, 3:10 PM Tim Rice <trice@posteo.net> wrote:

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