Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> * In the same vein, I don't know what this does:
>> sub pgbench($$$$$)
>> and I see no existing instances of it elsewhere in our tree. I think it'd
>> be better not to require advanced degrees in Perl programming in order to
>> read the test cases.
> It just says that 5 scalars are expected, so it would complain if "type"
> or number do not match. Now, why give type hints if they are not
> mandatory, as devs can always detect their errors by extensive testing
> instead:-)
My concern is basically about maintaining coding style consistency.
I don't want readers to come across something like this and have to
wonder "why is this function like this, when hundreds of apparently
similar functions elsewhere in the tree aren't? Is there some reason
why it needs to use this feature when the rest don't?". If the answer
to that is "yes" then there should be a comment explaining why. If
the answer is "no", well, it shouldn't be randomly different from the
rest of our code.
Now, having gone and looked up what that construct does, I wouldn't
necessarily be against a patch that introduced use of these poor man's
prototypes throughout our Perl code. It's the inconsistency that I'm
down on. But others with more Perl experience than I might have good
reasons why it's not like that already. I do have to say that many of
the examples I've seen look more like line noise than readable code.
>> * Another way in which command_checks_all introduces API complexity is
>> that it accounts for a variable number of tests depending on how many
>> patterns are provided. This seems like a mess. I see that you have
>> in some places (not very consistently) annotated call sites as to how
>> many tests they account for, but who's going to do the math to make
>> sure everything adds up?
> Perl:-) I run the test, figure out the number it found in the resulting
> error message, and update the number in the source. Not too hard:-)
Yeah, but then what error is all that work protecting you from?
regards, tom lane