[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Column labels with --full
Re: Column labels with --full
Mon, 6 Jun 2022 15:47:20 -0700
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.)
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
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.