Hi Yavor, I appreciate that you are providing a lot of feedback about this. It can only lead to further improving Scmbug.
On Mon, 2010-07-12 at 16:38 +0300, Yavor Nikolov wrote:
> OK - I agree sensitive code should be reviewed. My complaint was that
> we not only need to review them - but we need to change them.
> For example - I tried to "hack" your change about Version::create
> support for bugzilla 3_4:
> 1. I did the modifications in integration_add_tag
> 2. I added "sub is_version_up_to_3_4" and the relevant change in "sub
> set_version_type"
> * But this was not enough - since some other things stopped working
> until modified to accommodate the "up_to_3_4" addition.
I understand what you mean. The way I see it we are not changing them;
we are expressing that we have reviewed them. In practice, we add
something like
$self->is_version_up_to_3_4() ||
in some if statements. That's not changing the code inside the body of
the if.
> I.e. - I wanted to make the smallest possible change in order to get
> support for Bugzilla 3.4 in integration_add_tag. But in addition we
> had to also change:
> integration_change_bug_resolution
> integration_get_bugs
> init_specific
> And none of these methods refers to something in Bugzilla which has
> changed in v3.6 vs v3.4.
I think that's really your argument. You would have preferred if those
three functions contained checks like if (version >= 2.22 && version <=
3.2) then { ... }, since checks wouldn't need to be so explicit. I agree
that it could be simplified.
But I don't believe that it would save one from the effort of expressing
that that code section had been reviewed. You would still have to
inspect all such checks and where you had something like:
if (version >= 2.22 && version <= 3.2)
you would have to edit it to be
if (version >= 2.22 && version <= 3.4)
So, you would still have to edit it. The code expressed would be
shorter, but it doesn't save you from reviewing it and editing it. One
may propose it could be edited to just be:
if (version >= 2.22)
...which would not need editing in the future, but that would be
incorrect. The code now assumes it would work with higher versions than
what it has been tested with and will break with an incompatible higher
version.
> previous version. up_to_* currently means less_than_or_equal
> since the
> previous version. It seems to fit this model.
> Well:
> - If with each new Bugzilla releases most of the version-sensitive
> things change: then current approach is fine (we'll need to change
> these sections anyway).
> - If only a few things change: then makes sense to touch only these
> few places which are affected by that change (but not the others even
> though they have been changed in some previous releases).
>
> Currently in order to declare that some section applies for particular
> range of versions: we have some long expressions checking each of
> them.
I agree. But I believe that these expressions capture that there are
known configurations that are different. e.g. one may add a check like:
if (version <= 3.1.0)
but that would not express that we indeed know that's a known
misbehaving configuration; it could just be a type-o that means 3.1.2.
Instead, "up_to_3_1_2" expresses this is one of the 6-7 known, different
configurations we are reviewing.
> Calls to is_version_latest are also triggering some excessive changes
> since that's influenced by each new Bugzilla release which is gets
> supported by scmbug.
>
>
> I should admit things are not so complex as of today - the versions
> supported are not so many and the number of sensitive sections of code
> are not so many. But they're growing.
I agree that they are growing.
Can you provide a practical example, based on actual Scmbug:Bugzilla.pm
code of how you would like a check to look like ?
signature.asc
Description: This is a digitally signed message part
_______________________________________________ scmbug-users mailing list [email protected] http://lists.mkgnu.net/cgi-bin/mailman/listinfo/scmbug-users
