Eric Sunshine <sunsh...@sunshineco.com> writes:
>> +test_file_not_empty () {
>> + if ! test -s "$1"
>> + then
>> + echo "'$1' is not a non-empty file."
>
> Although not incorrect, the double-negative is hard to digest. I had
> to read it a few times to convince myself that it matched the intent
> of the new function. I wonder if a message such as
>
> echo "'$1' is unexpectedly empty"
>
> would be better. (Subjective, and not at all worth a re-roll.)
Yeah, that is subjective. The expectation of the test is "not-empty",
so I do not see this double-negation as being too bad, though.
> Much later in this same file, a function named test_must_be_empty() is
> defined, which is the complement of your new test_file_not_empty()
> function. The dissimilar names may cause confusion, so choosing a name
> more like the existing function might be warranted.
>
> Also, it might be a good idea to add this new function as a neighbor
> of test_must_be_empty() rather than defining it a couple hundred lines
> earlier in the file. Alternately, perhaps a preparatory patch could
> move test_must_be_empty() closer to the other similar functions
> (test_path_is_missing() and cousins).
Very good suggestions. Looking at neighbouring helpers around
must-be-empty, it seems to me that the latter, i.e. moving it to sit
next to other "path" helpers, would make the most sense.
Thanks.
