Robert Burrell Donkin ha scritto:
On Mon, Sep 15, 2008 at 2:04 PM, Stefano Bagnara <[EMAIL PROTECTED]> wrote:
Hi Robert,
I just did some review of the resulting imap product and I'm impressed.
it's still a little loose: the commons-lang dependency isn't
absolutely required by anything other than torque
org.apache.james.imapserver.codec.encode.EncoderUtils only includes a
encodeDateTime method and it uses
org.apache.commons.lang.time.FastDateFormat.
Otherwise it is only used by tests or by torque.
While I reviewed I created a graph for the current modules dependencies:
http://people.apache.org/~bago/imap/james-imap-module-dependencies.gif
Maybe this graph could be included in the website once we'll have one.
+1
they are really cool :-)
Thanks!
I have some question/comment:
1) Why do we have STATUSResponse and StatusResponse ?
IMAP specification
i've attempt to name objects from the specification but the
specification re-uses the term 'status'. there's quite a lot of
complexity which could usefully be pruned so hopefully one of these
can be eliminated.
My issue was mainly about the ALL-CAPS choice.
2) There are 2 package cycles:
imap.message.request.base <-> imap.message.request.imap4rev1
mailboxmanager.impl <-> mailboxmanager.util
as few as that?
yes, it seems :-)
this is an issue that i'd intended to come back and address post-M1
I'd like them to be removed:
2a) The imap.message.request could be mergerd into a single package (in base
there is an imap4rev1 class!).
2b) The mailboxmanage cycle is because of UidChangeTracker. Moving to to
"impl" should fix this too.
+1
dive right in
Done.
3) I see some module is really small (seda) while other modules are really
big (codec). I graphed the package dependency tree (it is a tree only after
grouping the 2 cycles above) and it resulted that the codec module have 2
separated hierarchies:
http://people.apache.org/~bago/imap/graph-imap-full-package-check-no-torque.gif
Does it worth splitting?
possibly
again, this is something i'd planned to address post-M1. M1 was
intended just to be a working IMAP for james, not anything more. the
code structure could do with revision.
Sure! I simply wanted to get more from my review process :-)
I'm not interested in imap ATM, but I find that if I keep reviewing
diffs I slowly loose the understanding of what we have and I may no more
be in-topic when I discuss or I cast my votes. I also wanted to see how
you dealt with socket testing without avalon.
(note that imap.message.request.base+imap4rev1 and
imap.command.imap4rev1 can be moved in both "groups").
3b) maybe we could even shorten package names: codec.encode => encode and
codec.decode => decode
+1
probably want to shorten imapserver -> imap as well
I thought you decided for imap when the code was not client/server
specific and imapserver when the code was for the server-side, but if
this is not the case maybe imap is better.
i'm not sure whether it's worthwhile including 'imap4rev1' in the
package since it's the only revision in common use
I think it should be removed. I don't like too much nested package trees.
3c) another option would be to extract the imap.message package to its own
module (message). codec (or both decode/encode if it is splitted) would then
depend on this message module). If I'm not wrong processor would so depend
only on "message" and not on codec(decode/encode).
a separate message module makes sense
this area is a little messy and hope it can be resolved more elegantly
once some of the deprecated structure's removed.
I created a message module but I also moved there the "encode" part of
the codec.
I did this temporarily because I don't know how you want to deal with
the ant api-library-function-deployment layering and the above
separation allowed me to not introduce another layer.
How would you handle this in the self imposed layer rule?
Maybe the whole "message" package should be moved to imap-api instead?
Looking again at it I don't fully understand why some of "imap.message"
should be in api and something else should be in codec (now message)
library. Can you give me any hint?
4) What about removing the "experimental" string from seda packages? I think
the "0.1" release version makes it already "experimental".
+1
I opened a JIRA assigned to me for this, will do another day.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
