Jakub Narebski <jnareb@xxxxxxxxx> writes:
> I like v2 version, and had only small corrections. For you to not have
> to resend this patch once again, I have added those corrections and sent
> them as a patch myself.
>
> Changes from original v2 version from Kevin Cernekee:
>
> * Fix (re-add) accidentally lost in v2 '"<td class=\"link\">"' in first
> chunk using href(-anchor => ...)
> * Reword commit message (hopefully make it better, and not only longer)
Thanks.
> * Move code that sets implicit -replay when -anchor is the only parameter
> lower, and change order of subexpression, for better possible future
> extendability, if there would be other cases when we would want to
> automatically turn on -replay.
I think you meant this part:
# implicit -replay
$params{-replay} = 1 if (keys %params == 1 && $params{-anchor});
But I don't think it is that much of an improvement as long as it uses the
statement modifier syntax ("A if B") for a thing like this.
I will not bother rewriting the commit I made out of this patch myself,
but the next person who wants to add the auto-replay for his option will
first have to rewrite the above into:
if (key %params == 1 && $params{-anchor}) {
$params{-replay} = 1;
}
before turning it into:
if ((key %params == 1 && $params{-anchor}) ||
("here comes my new condition")) {
$params{-replay} = 1;
}
anyway. If your rewrite were to a plain-vanilla "if () { ... }", it would
have been a real improvement.
Besides, I find it a bad taste to use statement modifier syntax unless the
modifying condition ("if B" part) is much more likely to hold true than
false.
If you limit your use of statement modifiers for conditions that almost
always hold true, it will become much easier to scan the code for the
first time, because you can skim through and almost ignore the condition
part to follow the logic for the normal case. But turning 'replay' on in a
specific narrow case (and later in a specific set of narrow cases) is a
complete opposite of normal codeflow.
Anyway, thanks for the clean-up. Applied.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html