On Tue, Sep 16, 2008 at 12:20 AM, Stefano Bagnara <[EMAIL PROTECTED]> wrote: > 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.
yes - it just requires someone sitting down and writing a suitable data parser. not a big job but i wanted to freeze the code until an M1 was cut. >>> 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. packaging might have been confusing but M1 is only intended to represent an intermediary phase of the code >>> 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 doesn't do real socket testing it might be worthwhile setting up some integration smoke tests which run against a full packaged default james server deployed for test. this would allow basic testing of deployed sockets. >>> (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. going forward, i think it'd be best for james server to use imapserver and the component to use imap >> 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. please dive in >>> 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. i'll take a look at this once you've finished > How would you handle this in the self imposed layer rule? no real need to keep the rule for IMAP when decomposing a project as large and coupled as james, the tendency is to create a mess of modules with deep and complex interdependencies rather than working to correctly separate concerns. the api/library/function split (plus deployment) prevents this from happening. in order to fit the modules into this constraint it's necessary to separate concerns and adopt a relatively coursely grained and simple system of coupling. > 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? i was planning to review this post M1 so it's probably that stuff's just badly organised >>> 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. great thanks for the hard work :-) - robert --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
