On Sun, Apr 18 2021, Eric Sunshine wrote:
> On Sat, Apr 17, 2021 at 8:58 AM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> wrote:
>> [...]
>> The goal here is to get rid of the verbosity of having e.g. a "test 2
>> -ne 2" line for every "test_cmp". We use "$@" as an argument to "test"
>> to intentionally feed the "test" operator too many arguments if the
>> functions are called with too many arguments, thus piggy-backing on it
>> to check the number of arguments we get.
>
> My one concern about this change is that it enters the realm of
> "huh?". Although it's dead-simple to understand what this code is
> doing:
>
> test "$#" -ne 1 && BUG "1 param"
> if ! test -f "$1"
>
> the replacement code:
>
> if ! test -f "$@"
>
> can easily lead to head-scratching, wondering why the author chose to
> use "$@" rather than the more obvious "$1". This sort of unusual local
> idiom might normally deserve an in-code comment explaining why the
> more obvious code is not employed. However, adding a comment at each
> site would be overkill, so it might be the sort of thing to explain in
> documentation somewhere ("In order to make -x output less noisy,
> employ "$@" rather than explicitly checking function arguments when
> writing new test functions...".) Otherwise, the reader is forced to
> consult the commit message.
>
> Not a major objection; just voicing the bit of unease I feel about it.
I'm including a well-placed comment in the re-roll of this. FWIW this
would be much easier to follow with my rejected split-up of
test-lib-functions.sh, then we could group these functions into one
file:
https://lore.kernel.org/git/20210209214159.22815-13-avarab@xxxxxxxxx/
>> This does not to the "right" thing in cases like:
>
> Channeling Dscho's inner Eric Sunshine[1]: s/to/do/
Sunshine recursion error? :)
> [1]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2103222235150.50@xxxxxxxxxxxxxxxxx/
