Stefano,
One question: The refactoring you are providing, does it work? (Did you
run it? Did you test all the features and functionality beforehand and
afterwards?)
I agree with Steve that a refactoring is not worth applying if we don't
know if it doesn't breaks something, regardeless how simple the
refactoring is, how ugly the code was and how shining it is afterwards.
I made this mistake too many times before myself.
Of course, we cannot catch every newly introduced bug at first commit,
but we should try. It's much less effort than hunt it anytime later.
Even if there _are_ unit tests, I see it as my duty as a committer do
run additional functional test by hand. Some things simply can't be done
with unit tests.
IMHO, if you find out that the touched code works like before or better,
go ahead and apply the patch.
And if you already did all this, please ignore my rant. ;-)
Bernd
Stefano Bagnara wrote:
I would like to add an example:
In James 2.1 we had FetchPOP
In James 2.2.0 FetchMail has been introduced and FetchPop deprecated.
Fetchmail was a complete rewrite and had no unit tests, and has been
introduced in a minor (2.#) release.
Does this mean that if I rename the refactored code to "GrabMail" we can
introduce it in James 2.4.0 and deprecate James 2.2.0 ? ;-)
I know that FetchMail was a big improvement over FetchPOP in the
features aspect, but I'm trying to add evidence to my reasons ;-)
I just want to note that if we require people to unittests every aspect
of James in order to change anything this could be the death of James.
The best effort for unit tests on James I ever seen has been done by
Bernd and we all really appreciated this. I also wrote as much unit
tests as possible when working on hard things (mainly MimeMessageWrapper
and the CoW proxy). I also wrote most of the unit tests in jSPF, because
I think tests are really usefull, but I don't think James will get any
new feature if the prerequisite is to write unit tests for the old code.
That said I don't want to enforce a bad practice only because that
practive has been followed in past, I just want to have a better
"measure" for our discussion.
And now a generic comment on this discussions:
What I expect from the PMC committers when anyone make a proposal is a
clean vote in the range -1...+1 with possible technical explanations,
alternative solutions, and blocking issues to change a -X vote to +0, if
any.
Imho if we want to release James much more than once in 2 years we have
to enforce a more agile decision making solution.
Stefano
Steve Brewin wrote:
My main issue is purely practical. I think it seriously unwise to make
changes on such a scale to any component without unit tests with
sufficient coverage to validate it still works across all usage the
scenarios. This is not a minor incrementl refactoring, of which I
fully approve. This is a total rewrite.
Without unit tests, I can't understand how we can have confidence in
such a rewrite when your main argument for doing so is that you do not
understand the existing code, "To me it's something like obfuscated: I
feel worst than browsing decompiled code ;-)".
This is not me being protective. This is me voicing me fears of
treading such a path for any component in a minor (2.#) release. As I
said, "We all know that major changes are prone to creating new
issues". I don't think any component with so few issues open against
it justifies the risk of introducing new bugs this degree of change
will inevitably entail.
I'm more than happy for you to prove me wrong by way of unit tests.
Otherwise I think this belongs in the bucket of things to consider for
3.0.
Cheers
-- Steve
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
