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 15:26:42 GMT
On Thu, 2010-01-07 at 15:32 +0100, Stefano Bagnara wrote:
> Sounds like a bikeshed issue. Of all big changes what you don't like
> are the 2 smaller changes :-)

My main issue is very simple: your changes were driven primarily by API
purism rather any practical benefit. Therefore I find it strange that
some very subjective API issues are fixed by introducing different API
ugliness such as modal API or abuse of class inheritance.

There is really a lot of good stuff in your branch, which I would love
to see merged to the trunk, but certain changes to the low level parser
classes are questionable, and I care mostly about low level mime4j

> >From your previous message where you talked about the fact that I
> introduced big functional changes I thought there was much more than
> this. These 2 are very minor changes that do not alter the way the
> library is used by users nor any functional behaviour.
> I'm sure that if the issues are only the 2 you list here we'll find a solution.
> more inline,
> 2010/1/7 Oleg Kalnichevski <olegk@apache.org>:
> > (1) Please revise / redo BasicMimeTokenStream / MimeTokenStream split or
> > revert 895241. Consider making MimeTokenStream an interface as an
> > option.
> 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.

I will work on an alternative approach in the coming days

> > (2) Please make sure  LineReaderInputStream#unread() does not impose a
> > particular mode of operation.
> This is easy to change. I still think this is worse (performance and
> memory wise) than the current solution. It is a really internal class
> that should not be used by users and, in the last revision, it already
> protects from multiple unread calls using exceptions. We use the
> "unread" method in a single place of our code (furthermore this
> already exists and works, other solutions should be implemented). If
> we don't want to optimize it at the extreme maybe we can switch the
> whole line reading behaviour to a Buffered Reader, so that we can
> simply use mark/reset. But even mark/resent impose a particular mode
> of operation. The fact is that we talk about buffers. If you don't
> want to impose limits then you need infinite memory :-)

See my main issue above.

> Stefano

View raw message