Steve Brewin wrote:
As you will have gathered, I don't think this is a good way of proposing a
major change in a component.
What would be a better way? I can't see anything better than the real
code to have concrete discussion.
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.
Well, until Bernd wrote the first tests James did not have unit tests
(well, maybe only few for IMAP) and James has been released for 4 years
with no unit tests.. btw I agree that unit tests are a big missing thing.
I don't think it's fair to say that the blocking issue is that unit test
are missing.
Btw, I don't care too much of the discussion itself, I want to find a
solution. Is this a veto? Will you retire the veto if I wrote unit tests?
This is not a total rewrite: I did it in 2 hours. I'm not good enought
to write a fetchmail implementation in 2 hours. Here are the refactoring
steps I did:
1) remove the set/get/compute/update/getBase pattern used for fields in
favor of a simple "lazy" get when possible
2) for each method I run a call hierarchy: if used only by a subclass,
move that method to the subclass, if it is not used remove the method.
3) process was defined in the abstract processor but apart the code
there was not the concept of AbstractProcessor in the code architecture:
nowhere the code was referring to the AbstractProcessor and not to
specific implementations. There was only 2 methods shared between the
processors while most of the code specific to specific implementations
was in the ProcessorAbstract: I removed the ProcessorAbstract in favor
of a cleanear solution, moving every needed method to the specific
implementation.
4) I refactored each processor to remove fields depending on Account or
on the specific item processed (Store/Folder/Message) and only using
reentrant methods.
5) I moved the Store/Folder/Message and the Account to process method
arguments so that a single processor could be used for multiple Accounts
and multiple resources if sharing the same ParsedConfiguration.
Submitted version1
6) I moved the processors creation to the initializing step, because we
there was no more the need to create a single processor for each message .
7) The code was already so much different from the original that I also
applied an automatic formatting (to match James spaces/brackets style)
Submitted version2
I agree this is a major change, but there is much difference between
major refactorings and introduction of new code or code rewritings.
I also would like to add that I don't think to be infallible and it is
probable that the refactoring introduce bugs: from my experience
"refatoring introduced bugs" are "easy bugs" (most times NPE) because
business logic is not involved.
I didn't even run the current refactored code, so maybe NPE are already
there and that James does not even start, but this is not the point of
this discussion: as I wrote it was a proposal and it is work in
progress. If we decide to follow that path I will do some more
work/check on it.
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 ;-)".
Yes, it is the main argument, and I want to underline (put emphasys)
that I still think that the performance and memory usage improvements I
did during the refactoring are much less important for the future of
James than the readability issue.
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.
Well, if the problem is only the version number then I don't care: we
can even use 4.0 if this allow me to work ;-)
Furthermore, I have concerns about the current reliability of fetchmail:
I used it only once and I experienced a blocking issue:
http://issues.apache.org/jira/browse/JAMES-359
In fact James 2.2.0 (our official release) is distributed with that
major bug but I only saw once or twice people on the mailing list
reporting that problem: what does this means? I hope this does not mean
that people using James 2.2.0 does not use fetchmail, because in this
scenario it is hard to say if the current fetchmail is reliable or not.
I never used fetchmail enough to understand its reliability on my own,
but I wonder how much people is using the current fetchmail feature set,
and, as an example, every mail attribute generated.
If I understand your words you are:
-1 to apply the code for a 2.4 release
but you don't apply a veto if the release will be 3.0 or if we add unit
tests, is it right?
If so I would prefer to apply the code now. When we will be ready for
the following release we'll look wether in the meantime anyone had time
to wrote unit tests or not and will decide to release a 2.4 or 3.0 based
on the unit tests presence.
This does not make much sense to me, but I only want a solution and I
consider version numbers only labels (I prefer to release a 3.0 early
with no much new features that never release a 2.4)
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
