[Top][All Lists]

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

Re: Column labels with --full

From: Tim Rice
Subject: Re: Column labels with --full
Date: Mon, 6 Jun 2022 22:10:12 +0000

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

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

reply via email to

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