james-mime4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oleg Kalnichevski <ol...@apache.org>
Subject Re: [cycleclean] branch review and questions
Date Thu, 07 Jan 2010 12:17:10 GMT
On Tue, 2010-01-05 at 20:33 +0100, Stefano Bagnara wrote:
> Hi all,
> 
> as you saw I worked on the cycleclean branch for some day.
> 
> If anyone have the time to review I'd like to collect thoughts on what
> I did, to understand if you like the direction or not, and, in case
> you like it, if you think that the branch should merge before
> releasing 0.7 or after. (IMHO it is already better than trunk and the
> whole refactoring should happen in trunk, but I'm happy to work in
> branches when I'm not able to describe my ideas with long
> discussions).
> 
> Please note that the branch should be considered work in progress
> (expecially wrt package names).
> 
> Also I have concerns about dom manipulation. In the branch I extracted
> the dom to 4 packages (message, field, field.address, field.datetime).
> As you see the dom has no dependency on other mime4j packages nor on
> commons-logging.
> 
> An user could build a message from scratch without parsing it using
> the DOM. Currently whenever the user adds a field, mime4j does
> something "weird"
> 1) create a raw representation of the field to be added (Field.someMethod)
> 2) lazily parse the raw representation to a ParsedField implementation.
> 3) if the user try to get back the data it is parsed again.
> Isn't it a bit "unintuitive"?
> 
> Then, the current lazy parsing and ParsedField interface "contract is:
> 1) you create a lazy parsed field, you never get an exception for this.
> 2) the first time you try to access a field property the field is
> lazily parsed. again you don't get notification for parsing issues,
> but simply a null value, like you get if the field did not have that
> specific parameter you are reading.
> 3) you have isValid() and getException methods to query the field
> object for parsing issues.
> Again, this doesn't seem intuitive to me...
> 
> I didn't worked out solutions yet, but I'd like to collect your
> opinions and ideas on this to avoid diverging too much from the
> community directions.
> 
> Stefano
> 

I am very sorry, Stefano, but I am not in favour of a _wholesale_ merge
of your changes. You have touched too many things in too many places.
The extent of changes is well beyond the initially stated goal and the
scope of the refactoring, which was meant to be elimination of cyclic
dependencies. You have introduced a number of significant _functional_
changes at the same time, which makes it difficult to understand the
intent of changes. Overall, I cannot help feeling that your efforts lost
focus. Things did not really get better, they just got different, more
to your personal taste. For instance, how does impl class
MimeTokenStream extending public class BasicMimeTokenStream make for a
better API, I just do not understand. So, cyclic dependencies are gone
but in return we have ended up having some really bizarre class
extensions. 

I personally prefer to have your changes merged in several iterations,
package by package, starting with low level stuff in 'parser' and 'io'
packages. Feel free to ignore me, though.

Oleg


Mime
View raw message