james-mime4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oleg Kalnichevski <ol...@apache.org>
Subject [cycleclean] minor naming tweaks; was Re: [cycleclean] branch review and questions
Date Fri, 08 Jan 2010 09:58:54 GMT
On Thu, 2010-01-07 at 16:45 +0100, Stefano Bagnara wrote:
> 2010/1/7 Oleg Kalnichevski <olegk@apache.org>:
> >> 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.


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.


View raw message