james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefano Bagnara <apa...@bago.org>
Subject Re: [jira] Updated: (JAMES-509) Cleanup/Refactor FetchMail code
Date Sat, 27 May 2006 13:16:58 GMT
Bernd Fondermann wrote:
> 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 already wrote I don't have run james with the refactored version: it 
takes time to do this things. I'm willing to do that if I don't have vetoes.

> 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.

I big -1 to this: *every* refactoring could potentially introduce a new 
bug. *Every* code change could potentially introduce bugs. The meter 
should not be that: the question should be does it worth the risk or not.
We may discuss wether this worth the risk or not, but I don't think that 
we can say "we don't know if this refactoring break something so we 
don't include it". This would be the same as "we don't accept 
refactorings in our process". Even if you have 100% coverage tests when 
you change a single char in your source code you can't be 100% *sure* 
you are not introducing new bugs.

> 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.

Please note that the attachment was only a proposal, work in progress, 
untested, only because I knew it could possibly receive vetoes. It is 
not complete work and I never tested it.
I simply said that the proposal took 2 hours: much less this discussion 
is taking, and a real concrete code to talk about, so I think it has 
been time well spent. If there are no vetoes on the approach, on the 
kind of change I will do some tests. I don't think we have to discuss 
this things. Do you really think I want to commit code randomly on the 
James source tree?

In past it happened I committed non-running code because I think that 
sometimes it is better to see the sequence of commit about a given 
refactoring than apply the final, tested result. When I'm 100% sure I 
can achieve a better result and I'm confident I will not receive vetoes 
I prefer this approach (even if this keep the trunk non-working for 1-2 
days), instead this time I preferred to use the RTC approach, so 
unfortunately there is no track of the small changes, but I thought that 
someone could not have liked if I committed the change and asked for review.

These behaviours are related to who works on the codebase: I almost 
worked an year "alone" on the James codebase, so it was not so important 
for me to not block other people work and so on.. I work also on 
projects with dozen of active committers and I use totally different 
practices.

> 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.

I agree.

> IMHO, if you find out that the touched code works like before or better, 
> go ahead and apply the patch.

It should work like or better than before. I cannot and never be 100% 
sure of this, but I think the risk worth the fact that I'll be able to 
manage that code later.

If I am the only one that has problem working on the fetchmail code I 
agree that we should not do the step, but I would also see someone else 
working on that code.

Stefano


---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message