Hi Kristis,
On Tue, Jul 13, 2010 at 01:31, Kristis Makris <[email protected]> wrote:
> 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.
>
Yes, I meant something like that.
>
> 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)
>
Semantics here are not much different than the "else" clause which we
already have:
...
} elsif ( $self->is_version_up_to_2_22() ||
$self->is_version_up_to_3_1_2() ) {
...
} else {
...
}
which we already have.
>
> ...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.
>
Reviewing - OK. But I'm against using those long expressions to just denote
we've reviewed it. (Something else - e.g. temporary comments may be used
instead in order to track our review progress inline).
We'll need to review all calls to Bugzilla where a change has been
introduced to it. Just addition one more "is_version_up_to_***" expression
doesn't guarantee the given section has been modified accordingly nor that
it has been tested and compatible with all the versions.
I suspect for most new bugtracker releases many of the things will actually
remain stable and won't need modification in future versions. So expressions
like "if (version >= 2.22)" or "is_version_between( $VERSION_2_22,
$VERSION_LATEST )" seem OK for me.
>
> > 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.
>
To avoid typos we may use predefined constants for version numbers; and also
we may have dynamic checks for parameters' validity in a function like
is_version_between( a, b ).
>
> > 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 ?
>
Sure - I'll try to make some working example these days. Here is some
fraction here:
sub init_specific {
...
if ($self->is_version_up_to_3_4() || $self->is_version_latest()) {
unshift @INC, $self->installation_directory() . "/lib";
}
...
}
to be changed to:
sub init_specific {
...
if ($self->is_version_between( $VERSION_3_4_0, $VERSION_LATEST )) {
unshift @INC, $self->installation_directory() . "/lib";
}
...
}
That's my current idea (maybe there is a better approaches). The main goal
is to ease maintenance and improve code readability, especially when adding
support for new bugtracker release.
Regards,
Yavor
_______________________________________________
scmbug-users mailing list
[email protected]
http://lists.mkgnu.net/cgi-bin/mailman/listinfo/scmbug-users
