On Thu, 2010-01-07 at 16:45 +0100, Stefano Bagnara wrote: > 2010/1/7 Oleg Kalnichevski <[email protected]>: > >> All I need is to have a dependency free token stream parser. If you > >> can suggest a better option that extracting the BaseMimeTokenStream > >> abstract class I'm fine with it. While I may like introducing an > >> interface, I fail to see how it fixes any issue here. Can you go in > >> details? (I don't understand why the abstract class hurts you so > >> much..) > > > > Because it is not abstract to start with and is called Basic* not Base*. > > I simply do not understand the distinction between the two classes from > > the functional standpoint. Why one concrete class has been thrown into a > > public api package and another one into the impl package. This just does > > not sound right. > > If you have any better name you're welcome! I usually don't care too > much about names at this step of my refactorings. So I agree that most > class names and most package names can be fixed (I tried to change as > few as I could to help reviewing, and when I introduced new names I > chose the one that made sense to me at that moment, nothing written in > stone). > > The Basic parser is a parser that know nothing about field parser > implementations, it simply contains the basic logic. You can use it if > you need your own BodyDescriptor and the 2 basic implementations are > not enough for you, or if you want to use the parser without the whole > field dependency. >
Stefano, I spent more time working with your code. The refactoring of MimeTokenStream makes good sense. However, the unfortunate choice of package and class names sent a completely wrong message. It led me to believe one package was meant to represent public API with impl package being implementation of that API. If the following changes can be made to package / class naming I can happily vote +1 to merging down the entire branch. Smaller issues such as #unread() method modality can be dealt with on the trunk. option 1) o.a.j.mime4j.parser -> o.a.j.mime4j.stream o.a.j.mime4j.parser.impl -> o.a.j.mime4j.parser impl.MimeTokenStream -> MaximalMimeTokenStream option 2) o.a.j.mime4j.parser.impl -> o.a.j.mime4j.parser.max impl.MimeTokenStream -> max.MaximalMimeTokenStream I also played around with idea of the iterative merge and found it pretty doable. I merged down low level packages util, io, parser, codec and managed to tweak higher level packages to compile against the new parser API. Merging your code in stages would make it possible for people with better knowledge of higher level API review the changes without being distracted by changes in low level stuff. I am perfectly aware of downsides of this approach, such conflict resolution pains, but still feel it could help build a consensus among all committers. Oleg
