dom / message refactoring; was Re: 0.6.2 or 0.7 any time soon?

[Oleg Kalnichevski <ol...@apache.org>]

On Tue, 2011-01-11 at 21:13 +0100, Oleg Kalnichevski wrote:

...

> 
> Stefano, 
> 
> I already explained why I no longer feel comfortable committing to
> trunk. There is no point repeating that. 
> 
> > We will ask questions when we need answers :-)
> > 
> > Stefano
> > 
> > PS: if something I said/did makes you angry just explain me please. I
> > hope this is just something related to "translation" and the fact we
> > don't speak the same language natively.
> > 
> 
> I am not angry. I just find it regrettable that the project was
> effectively paralyzed for a full year.
> 
> I'll start working on a branch. 
> 
> Cheers
> 
> Oleg 
> 

Stefano et al

Please review another round of API changes on the refactoring branch.
Here is the gist of most important changes

* Most of the object model implementation logic moved from 'dom' to
'message'. 'dom' consists mostly of pure interfaces now that do not
impose particular internal structures, a particular implementation or a
particular inheritance hierarchy. 

* All dom element copy / parse logic moved from individual classes to a
utility class currently called MimeBuilder (I would prefer this class to
be called MessageBuilder but this name is already taken)

Please let me know if you can live with these changes and whether it is
ok to merge them down to trunk.

Oleg

Re: MessageBuilder refactoring; was Re: dom / message refactoring

[Norman Maurer <no...@apache.org>]

2011/1/26 Stefano Bagnara <ap...@bago.org>:
> 2011/1/26 Oleg Kalnichevski <ol...@apache.org>:
>>
>>> The fact is that probably ServiceLoader was required in the previous
>>> design, using Abstract classes. That design is the only I'm aware of
>>> that would have allowed future releases to add more methods to each
>>> class without breaking compatibility with older sources. Now that you
>>> removed this possibility by switching everything to interfaces I
>>> understand that the ServiceLoader seems "horrid". Most stuff I did in
>>> the dom package has been done with this precise goal, so I can't
>>> "defend" that code if we remove the goal :-)  That said you also
>>> should understand that the ServiceLoader, like most SPI model out
>>> there, is simply an abstraction layer. You are free to directly
>>> instantiate the implementation.
>>>
>>>
>>> I didn't (and I currently don't) know a better way to create a forward
>>> compatible, yet extensible, API, but I'm far from being an expert API
>>> designer.
>>>
>>
>> Stefano
>>
>> I am sorry I do not see how the service locator pattern can help with
>> maintaining forward compatibility between releases. I always thought
>> that it's goal was primarily to decouple layers and to enable multiple
>> implementations of a service interface.
>>
>> http://en.wikipedia.org/wiki/Service_locator_pattern
>
> I wrote "the ServiceLoader, ..., is simply an abstraction layer." so
> I'm in line with wikipedia, I'm sorry if I mislead you with some other
> sentence.
>
> I try to explain with other words:
> - abstract model was there to allow forward compatibility (it is the
> only way I know that allows adding a method in the model without
> breaking everything)
> - service locator adds "indirection" (decoupling)
> You need both if you want to "publish" a single package and you don't
> want this package to have direct dependencies on the implementation.
> The 2 patterns are extensively used in the standard java class library
> and in many  common libraries (I can't take the merit for mixing them
> ;-) ).
>
> I'm not sure I understand what part of the ServiceLocator is "horrid"
> in your opinion (I don't want to force you explaining anything or to
> waste your time, but I'm always interested learning from other
> developers).
>
>>> > How do you want me to proceed now?
>>>
>>> I'm happy to see you pushing. *Go* *ahead* :-)  I just explained why I
>>> did things in a given way and how I evaluate changes. This was not a
>>> comment to stop you, but just something to let you be more informed.
>>> As long as 0.7 will support the Monitor object, the fixed headless
>>> parsing and the fixed field folding I will be happy (I currently have
>>> 0.7-SNAPSHOT as dependency for this and not for the DOM abstraction).
>>>
>>> Stefano
>>
>> I do not want to 'push' if there is no consensus that I am pushing in
>> the right direction.
>
> You should ;-)
> - You have the time to prepare a next release
> - You are a project committer
> - No one is vetoing your changes
>
> The fact that I prefer the "previous" design and I don't see critical
> "inconsistencies" in that design is just a comment (an IMHO). I accept
> to loose that stuff as in turn you'll give me back a release soon. So
> I think for both me and the community having a release with the many
> bugfix/features introduced in trunk is a big win. (hope this makes
> sense)
>
> Stefano
>

+1 Let's keep pushing for a new mime4j release :)

Bye,
Norman

Re: MessageBuilder refactoring; was Re: dom / message refactoring

[Stefano Bagnara <ap...@bago.org>]

2011/1/26 Oleg Kalnichevski <ol...@apache.org>:
> [...]
> Fine. The problem is that Service Locator is considered an anti pattern
> by many, an opinion I fully subscribe to.

I don't have a strong opinion on this. IMHO it depends on the context
(also, an antipattern is usually compared to a better alternative to
reach the same goal).

If you want to remove it from svn feel free to do that. I just care
about the operational fixes/improvements I did there and don't care
about the locator or the dom api (sooner or later it would be good to
have this api, but it is not an high priority IMO).

Consider it a test. I did it in trunk simply because no one else was
working there and we usually prefer coding to happen in trunk (in fact
the locator+factories is something that took few hours, nothing more
than an experiment and we probably wasting too much time "discussing"
it when it can either be improved or replaced by alternatives)

I'm not against removing the locator, removing the "abstract classes
style" and so on (and you should feel free doing this, really).

The main "design" thing I care is about package dependencies: I don't
want dependency cycles between packages (I know some developers don't
care about this, but in my experience it is very important and I try
to defend this as I can: I'm aware people think I'm extremist about
this).

> [...]
> What is wrong with plain old java classes? What is it _exactly_ that was
> wrong with Message classes in 0.6?

goals in my mind when I did the change:
1) having a single package exposing dom functionality/classes so
client classes simply have "import mime4j.dom.*;" and not some import
from parser some from stream some from message and some from field.
2) make sure this package had no direct dependencies on the parsing
package (DOM should not depend on a parsing implementation: DOM is
about structure, not parsing)
3) allow future release to be able to add new methods/behaviours
without breaking compatibilty.

I remember 0.6 had many other issue I had to refactor in order to come
up with clean dependency tree and fix some bug regarding consistency
(headless parsing, header folding, empty lines), but if you want to
backport fixes and keep the "Message" style from 0.6 I can live with
it too. I'm sorry but my memory don't let me remember all of the issue
I saw in 0.6 (I don't even remember what I ate yesterday). Another
motivation was using an API java developers were used to (Most java
developers used/understand xml api so it was a good match).

> I am not sure. Now we have a situation when neither you nor I like the
> existing DOM API that much.

If I understand the way you liked the 0.6 version then it should be
simple to make you like trunk again.
1) Just leave interfaces in dom.
2) Remove the factories and service locator from dom (or ignore they
exists, they are just something "additional" you are not forced to use
them).
3) Tell people to instantiate Message implementation from message
package (MimeMessage / MessageImpl).

How is so much different from 0.6 with dom interfaces extracted to
another package?
Are you instead saying you don't want the "dom" package at all?

While I try to remember something else from the upgrade of my "client
code" from 0.6 to 0.7-SNAPSHOT I personally prefer the way some
configuration is exposed via the new factories instead of the 0.6
method of passing a MimeEntityConfig whenever you wanted to alter the
default behaviour (I think config objects like this are unexpected
from a java library user).

jDKIM with mime4j 0.6:
http://svn.apache.org/viewvc/james/jdkim/trunk/main/src/main/java/org/apache/james/jdkim/impl/Message.java?revision=824254&view=markup
You can see I had to extend MimeTokenStream  just to be able to
configure a new max line length.
Also I had to use the low level token stream for a simple task like
parsing message headers.

jDKIM with mime4j 0.7-SNAP (didn't update to the current trunk, it is
still against trunk before you worked on it):
http://svn.apache.org/viewvc/james/jdkim/trunk/main/src/main/java/org/apache/james/jdkim/impl/Message.java?revision=998320&view=markup
It still uses MimeEntityConfig but you can see the comment where I was
planning to have similar configurations exposed as factory properties:
--
MessageBuilderFactory mbf = MessageBuilderFactory.newInstance();
68 	mbf.setAttribute("MimeEntityConfig", mec);
69 	// mbf.setProperty("MaxLineLength", 10000);
---
Once MaxLineLength was a factory configuration property the jDKIM
would be using only dom classes (and not message/parser/stream lower
level packages).

Also the code moved from 184 to 130 LOC.

Unfortunately my experience with mime4j <=0.6 is limited to jdkim
because the only other project where I used mime4j used 0.7-SNAP since
the first version because I use the validation and the Monitor to
report errors found while parsing (not available in 0.6).

> Anyway, I'll stick to my original plan, bring mime4j to a state in which
> it could be released as 0.7 and step back again.

OK (and feel free to drop my trunk tests if you have ideas, really)

Stefano

PS: This long messages are not to "defend" my opinion, but just to
explain trunk code and historical steps.

Re: MessageBuilder refactoring; was Re: dom / message refactoring

[Oleg Kalnichevski <ol...@apache.org>]

> > I am sorry I do not see how the service locator pattern can help with
> > maintaining forward compatibility between releases. I always thought
> > that it's goal was primarily to decouple layers and to enable multiple
> > implementations of a service interface.
> >
> > http://en.wikipedia.org/wiki/Service_locator_pattern
> 
> I wrote "the ServiceLoader, ..., is simply an abstraction layer." so
> I'm in line with wikipedia, I'm sorry if I mislead you with some other
> sentence.
> 

Fine. The problem is that Service Locator is considered an anti pattern
by many, an opinion I fully subscribe to.


> I try to explain with other words:
> - abstract model was there to allow forward compatibility (it is the
> only way I know that allows adding a method in the model without
> breaking everything)


What is wrong with plain old java classes? What is it _exactly_ that was
wrong with Message classes in 0.6?


> I'm not sure I understand what part of the ServiceLocator is "horrid"
> in your opinion (I don't want to force you explaining anything or to
> waste your time, but I'm always interested learning from other
> developers).
> 

See above.


> The fact that I prefer the "previous" design and I don't see critical
> "inconsistencies" in that design is just a comment (an IMHO). I accept
> to loose that stuff as in turn you'll give me back a release soon. So
> I think for both me and the community having a release with the many
> bugfix/features introduced in trunk is a big win. (hope this makes
> sense)
> 

I am not sure. Now we have a situation when neither you nor I like the
existing DOM API that much.

Anyway, I'll stick to my original plan, bring mime4j to a state in which
it could be released as 0.7 and step back again.

Oleg

Re: MessageBuilder refactoring; was Re: dom / message refactoring

[Stefano Bagnara <ap...@bago.org>]

2011/1/26 Oleg Kalnichevski <ol...@apache.org>:
>
>> The fact is that probably ServiceLoader was required in the previous
>> design, using Abstract classes. That design is the only I'm aware of
>> that would have allowed future releases to add more methods to each
>> class without breaking compatibility with older sources. Now that you
>> removed this possibility by switching everything to interfaces I
>> understand that the ServiceLoader seems "horrid". Most stuff I did in
>> the dom package has been done with this precise goal, so I can't
>> "defend" that code if we remove the goal :-)  That said you also
>> should understand that the ServiceLoader, like most SPI model out
>> there, is simply an abstraction layer. You are free to directly
>> instantiate the implementation.
>>
>>
>> I didn't (and I currently don't) know a better way to create a forward
>> compatible, yet extensible, API, but I'm far from being an expert API
>> designer.
>>
>
> Stefano
>
> I am sorry I do not see how the service locator pattern can help with
> maintaining forward compatibility between releases. I always thought
> that it's goal was primarily to decouple layers and to enable multiple
> implementations of a service interface.
>
> http://en.wikipedia.org/wiki/Service_locator_pattern

I wrote "the ServiceLoader, ..., is simply an abstraction layer." so
I'm in line with wikipedia, I'm sorry if I mislead you with some other
sentence.

I try to explain with other words:
- abstract model was there to allow forward compatibility (it is the
only way I know that allows adding a method in the model without
breaking everything)
- service locator adds "indirection" (decoupling)
You need both if you want to "publish" a single package and you don't
want this package to have direct dependencies on the implementation.
The 2 patterns are extensively used in the standard java class library
and in many  common libraries (I can't take the merit for mixing them
;-) ).

I'm not sure I understand what part of the ServiceLocator is "horrid"
in your opinion (I don't want to force you explaining anything or to
waste your time, but I'm always interested learning from other
developers).

>> > How do you want me to proceed now?
>>
>> I'm happy to see you pushing. *Go* *ahead* :-)  I just explained why I
>> did things in a given way and how I evaluate changes. This was not a
>> comment to stop you, but just something to let you be more informed.
>> As long as 0.7 will support the Monitor object, the fixed headless
>> parsing and the fixed field folding I will be happy (I currently have
>> 0.7-SNAPSHOT as dependency for this and not for the DOM abstraction).
>>
>> Stefano
>
> I do not want to 'push' if there is no consensus that I am pushing in
> the right direction.

You should ;-)
- You have the time to prepare a next release
- You are a project committer
- No one is vetoing your changes

The fact that I prefer the "previous" design and I don't see critical
"inconsistencies" in that design is just a comment (an IMHO). I accept
to loose that stuff as in turn you'll give me back a release soon. So
I think for both me and the community having a release with the many
bugfix/features introduced in trunk is a big win. (hope this makes
sense)

Stefano

Re: MessageBuilder refactoring; was Re: dom / message refactoring

[Oleg Kalnichevski <ol...@apache.org>]

> The fact is that probably ServiceLoader was required in the previous
> design, using Abstract classes. That design is the only I'm aware of
> that would have allowed future releases to add more methods to each
> class without breaking compatibility with older sources. Now that you
> removed this possibility by switching everything to interfaces I
> understand that the ServiceLoader seems "horrid". Most stuff I did in
> the dom package has been done with this precise goal, so I can't
> "defend" that code if we remove the goal :-)  That said you also
> should understand that the ServiceLoader, like most SPI model out
> there, is simply an abstraction layer. You are free to directly
> instantiate the implementation.
>
>
> I didn't (and I currently don't) know a better way to create a forward
> compatible, yet extensible, API, but I'm far from being an expert API
> designer.
> 

Stefano

I am sorry I do not see how the service locator pattern can help with
maintaining forward compatibility between releases. I always thought
that it's goal was primarily to decouple layers and to enable multiple
implementations of a service interface.   

http://en.wikipedia.org/wiki/Service_locator_pattern


> >
> > How do you want me to proceed now?
> 
> I'm happy to see you pushing. *Go* *ahead* :-)  I just explained why I
> did things in a given way and how I evaluate changes. This was not a
> comment to stop you, but just something to let you be more informed.
> As long as 0.7 will support the Monitor object, the fixed headless
> parsing and the fixed field folding I will be happy (I currently have
> 0.7-SNAPSHOT as dependency for this and not for the DOM abstraction).
> 
> Stefano
> 

I do not want to 'push' if there is no consensus that I am pushing in
the right direction.

Oleg 

Re: MessageBuilder refactoring; was Re: dom / message refactoring

[Markus Wiederkehr <ma...@gmail.com>]

On Tue, Jan 25, 2011 at 10:09 PM, Stefano Bagnara <ap...@bago.org> wrote:
> 2011/1/25 Oleg Kalnichevski <ol...@apache.org>:
>> On Tue, 2011-01-25 at 17:49 +0100, Stefano Bagnara wrote:
>>> 2011/1/25 Oleg Kalnichevski <ol...@apache.org>:
>>> > * MessageBuilder redesigned and changed to an interface
>>> > * Moved #writeTo method to MessageWriter interface
>>>
>>> Moving "writeTo" from each "DOM node" to a MessageWriter interface
>>> have the drawback that you cannot anymore serialize just part of a
>>> message (just a subtree, an header, a content) because your
>>> MessageWriter serializes only full messages. On the other side I see
>>> the advantage of having the writer separated from the element itself
>>> (reading and writing are different tasks).
>>>
>>
>> I am sorry but that was not done by me.
>>
>> http://svn.apache.org/viewvc?view=revision&revision=739822
>
> So it seems the "feature" has been removed long ago! Thanks for the link.

It is still possible to serialize parts of a Message because
MessageWriter (MimeWriter in Oleg's branch) publicly exposes the
methods to do so (writeMultipart(), etc).

See http://svn.apache.org/viewvc/james/mime4j/trunk/dom/src/main/java/org/apache/james/mime4j/message/MessageWriter.java?view=markup

IIRC the reason for that change was that there were different
strategies for writing messages (strict, lenient) and before the
change the code for writing and these strategies in particular was
scattered around the various message classes (Header, Entity,
Multipart, ..). Now it's all in one place. See also MIME4J-110..

Later, in the course of MIME4J-118, the different writing modes were
removed and now there is only one MessageWriter instance called
DEFAULT.

Cheers,
Markus

Re: MessageBuilder refactoring; was Re: dom / message refactoring

[Stefano Bagnara <ap...@bago.org>]

2011/1/25 Oleg Kalnichevski <ol...@apache.org>:
> On Tue, 2011-01-25 at 17:49 +0100, Stefano Bagnara wrote:
>> 2011/1/25 Oleg Kalnichevski <ol...@apache.org>:
>> > ...
>> >
>> >>
>> >> >> [...] BTW I'm just loud thinking, just go ahead with
>> >> >> your plan :-)
>> >> >
>> >> > I am going to get there shortly. I just prefer to make potentially
>> >> > contentious changes in small, incremental steps, so that they are easier
>> >> > to review and agree / disagree upon.
>> >>
>> >> I understand. I'm impatient to see your changes :-)
>> >>
>> >> Happy hacking
>> >> Stefano
>> >>
>> >
>> > Stefano et al
>> >
>> > I just completed refactoring of MessageBuilder and related classes on
>> > the refactoring branch. Here is a short summary of changes:
>> >
>> > * MessageBuilder redesigned and changed to an interface
>> > * Moved #writeTo method to MessageWriter interface
>>
>> Moving "writeTo" from each "DOM node" to a MessageWriter interface
>> have the drawback that you cannot anymore serialize just part of a
>> message (just a subtree, an header, a content) because your
>> MessageWriter serializes only full messages. On the other side I see
>> the advantage of having the writer separated from the element itself
>> (reading and writing are different tasks).
>>
>
> I am sorry but that was not done by me.
>
> http://svn.apache.org/viewvc?view=revision&revision=739822

So it seems the "feature" has been removed long ago! Thanks for the link.

>> I haven't carefully looked at the code to see if you preserved some of
>> the optimizations about unparsed content streamed as-is in writeTo
>> instead of being recreated (this was good for performances and also to
>> keep input formatting when writing out a parsed message with no
>> changes).
>>
>> > * MessageBuilderFactory renamed to MessageServiceFactory and can now be
>> > used to instantiate message writers and builders. Additional factory
>> > methods can be introduced in the future.
>> >
>> > Please review and let me know if you can live with these changes.
>>
>> I hoped you wanted to deal also with DOM altering methods as I thought
>> you was not happy with dom mainly because it was an incomplete api (so
>> I probably didn't understand you well). I don't see "improvements"
>> over previous trunk
>
> My main problem was the lack of consistency in your code more than
> anything else. I never claimed I was able to create a "better" /
> "enhanced" / "more complete" API. I was merely trying to resolve
> inconsistencies while preserving the initial design and the general
> approach that you had started. If I had my way I would simply throw away
> every single class in the 'dom' packages, especially that horrid
> ServiceLoader, and reverted back to the simpler Message API that had
> existed before.

The fact is that probably ServiceLoader was required in the previous
design, using Abstract classes. That design is the only I'm aware of
that would have allowed future releases to add more methods to each
class without breaking compatibility with older sources. Now that you
removed this possibility by switching everything to interfaces I
understand that the ServiceLoader seems "horrid". Most stuff I did in
the dom package has been done with this precise goal, so I can't
"defend" that code if we remove the goal :-)  That said you also
should understand that the ServiceLoader, like most SPI model out
there, is simply an abstraction layer. You are free to directly
instantiate the implementation.

I didn't (and I currently don't) know a better way to create a forward
compatible, yet extensible, API, but I'm far from being an expert API
designer.

>> (But maybe this is mainly a matter of style /
>> different approaches to API design), but it is anyway much better than
>> 0.6 so I can live with it (power to active people).
>>
>> To avoid misunderstandings: I'm not "sad" accepting this changes, on
>> the contrary I'm happy to see someone pushing mime4j again and I just
>> try to find the time to review and explain why things were done in
>> that way.
>>
>
> How do you want me to proceed now?

I'm happy to see you pushing. *Go* *ahead* :-)  I just explained why I
did things in a given way and how I evaluate changes. This was not a
comment to stop you, but just something to let you be more informed.
As long as 0.7 will support the Monitor object, the fixed headless
parsing and the fixed field folding I will be happy (I currently have
0.7-SNAPSHOT as dependency for this and not for the DOM abstraction).

Stefano

Re: MessageBuilder refactoring; was Re: dom / message refactoring

[Oleg Kalnichevski <ol...@apache.org>]

On Tue, 2011-01-25 at 17:49 +0100, Stefano Bagnara wrote:
> 2011/1/25 Oleg Kalnichevski <ol...@apache.org>:
> > ...
> >
> >>
> >> >> [...] BTW I'm just loud thinking, just go ahead with
> >> >> your plan :-)
> >> >
> >> > I am going to get there shortly. I just prefer to make potentially
> >> > contentious changes in small, incremental steps, so that they are easier
> >> > to review and agree / disagree upon.
> >>
> >> I understand. I'm impatient to see your changes :-)
> >>
> >> Happy hacking
> >> Stefano
> >>
> >
> > Stefano et al
> >
> > I just completed refactoring of MessageBuilder and related classes on
> > the refactoring branch. Here is a short summary of changes:
> >
> > * MessageBuilder redesigned and changed to an interface
> > * Moved #writeTo method to MessageWriter interface
> 
> Moving "writeTo" from each "DOM node" to a MessageWriter interface
> have the drawback that you cannot anymore serialize just part of a
> message (just a subtree, an header, a content) because your
> MessageWriter serializes only full messages. On the other side I see
> the advantage of having the writer separated from the element itself
> (reading and writing are different tasks).
> 

I am sorry but that was not done by me.

http://svn.apache.org/viewvc?view=revision&revision=739822


> I haven't carefully looked at the code to see if you preserved some of
> the optimizations about unparsed content streamed as-is in writeTo
> instead of being recreated (this was good for performances and also to
> keep input formatting when writing out a parsed message with no
> changes).
> 
> > * MessageBuilderFactory renamed to MessageServiceFactory and can now be
> > used to instantiate message writers and builders. Additional factory
> > methods can be introduced in the future.
> >
> > Please review and let me know if you can live with these changes.
> 
> I hoped you wanted to deal also with DOM altering methods as I thought
> you was not happy with dom mainly because it was an incomplete api (so
> I probably didn't understand you well). I don't see "improvements"
> over previous trunk 

My main problem was the lack of consistency in your code more than
anything else. I never claimed I was able to create a "better" /
"enhanced" / "more complete" API. I was merely trying to resolve
inconsistencies while preserving the initial design and the general
approach that you had started. If I had my way I would simply throw away
every single class in the 'dom' packages, especially that horrid
ServiceLoader, and reverted back to the simpler Message API that had
existed before.


> (But maybe this is mainly a matter of style /
> different approaches to API design), but it is anyway much better than
> 0.6 so I can live with it (power to active people).
> 
> To avoid misunderstandings: I'm not "sad" accepting this changes, on
> the contrary I'm happy to see someone pushing mime4j again and I just
> try to find the time to review and explain why things were done in
> that way.
> 

How do you want me to proceed now?

Cheers,

Oleg

Re: MessageBuilder refactoring; was Re: dom / message refactoring

[Stefano Bagnara <ap...@bago.org>]

2011/1/25 Oleg Kalnichevski <ol...@apache.org>:
> ...
>
>>
>> >> [...] BTW I'm just loud thinking, just go ahead with
>> >> your plan :-)
>> >
>> > I am going to get there shortly. I just prefer to make potentially
>> > contentious changes in small, incremental steps, so that they are easier
>> > to review and agree / disagree upon.
>>
>> I understand. I'm impatient to see your changes :-)
>>
>> Happy hacking
>> Stefano
>>
>
> Stefano et al
>
> I just completed refactoring of MessageBuilder and related classes on
> the refactoring branch. Here is a short summary of changes:
>
> * MessageBuilder redesigned and changed to an interface
> * Moved #writeTo method to MessageWriter interface

Moving "writeTo" from each "DOM node" to a MessageWriter interface
have the drawback that you cannot anymore serialize just part of a
message (just a subtree, an header, a content) because your
MessageWriter serializes only full messages. On the other side I see
the advantage of having the writer separated from the element itself
(reading and writing are different tasks).

I haven't carefully looked at the code to see if you preserved some of
the optimizations about unparsed content streamed as-is in writeTo
instead of being recreated (this was good for performances and also to
keep input formatting when writing out a parsed message with no
changes).

> * MessageBuilderFactory renamed to MessageServiceFactory and can now be
> used to instantiate message writers and builders. Additional factory
> methods can be introduced in the future.
>
> Please review and let me know if you can live with these changes.

I hoped you wanted to deal also with DOM altering methods as I thought
you was not happy with dom mainly because it was an incomplete api (so
I probably didn't understand you well). I don't see "improvements"
over previous trunk (But maybe this is mainly a matter of style /
different approaches to API design), but it is anyway much better than
0.6 so I can live with it (power to active people).

To avoid misunderstandings: I'm not "sad" accepting this changes, on
the contrary I'm happy to see someone pushing mime4j again and I just
try to find the time to review and explain why things were done in
that way.

Thank you,
Stefano

MessageBuilder refactoring; was Re: dom / message refactoring

[Oleg Kalnichevski <ol...@apache.org>]

...

> 
> >> [...] BTW I'm just loud thinking, just go ahead with
> >> your plan :-)
> >
> > I am going to get there shortly. I just prefer to make potentially
> > contentious changes in small, incremental steps, so that they are easier
> > to review and agree / disagree upon.
> 
> I understand. I'm impatient to see your changes :-)
> 
> Happy hacking
> Stefano
> 

Stefano et al

I just completed refactoring of MessageBuilder and related classes on
the refactoring branch. Here is a short summary of changes: 

* MessageBuilder redesigned and changed to an interface
* Moved #writeTo method to MessageWriter interface
* MessageBuilderFactory renamed to MessageServiceFactory and can now be
used to instantiate message writers and builders. Additional factory
methods can be introduced in the future.

Please review and let me know if you can live with these changes.

I am _almost_ done. As the very last step I would like to try to
decouple 'message' and 'storage' classes, but I could also live with
things in their present form.

Oleg

Re: dom / message refactoring; was Re: 0.6.2 or 0.7 any time soon?

[Stefano Bagnara <ap...@bago.org>]

2011/1/18 Oleg Kalnichevski <ol...@apache.org>:
> I understand the downside of having a pure interface based API. However,
> you seem to be very keen to position mime4j not only as a utility for
> parsing and building mime messages but also as an abstract Document
> Object Model with different / alternative implementations. I just do not
> see how one can honestly call the existing code in trunk an abstract
> Object Model if the only aspect that is abstract is field parsing. We
> should either drop the pretense at having an abstract DOM in mime4j and
> revert to the state existed before your changes or actually make an
> effort to define a truly abstract model even if that means more effort.
>
> I am very fine with just reverting to the old state, by the way.
>
> So, what shall I do?

Go ahead with your plan.

I agree that DOM is incomplete now.

I don't think that we'll see alternative implementations, but having a
good DOM api is also a way to stabilize an interface and put a clear
limit in what can be changed and what cannot be changed.

I would like before 1.0 to tell people feel free to refer .dom.
classes, don't touch .field./.message./.storage./ .

Having no direct dependencies from dom to the implementation is a way
to be sure this happen.

>> [...] BTW I'm just loud thinking, just go ahead with
>> your plan :-)
>
> I am going to get there shortly. I just prefer to make potentially
> contentious changes in small, incremental steps, so that they are easier
> to review and agree / disagree upon.

I understand. I'm impatient to see your changes :-)

Happy hacking
Stefano

Re: dom / message refactoring; was Re: 0.6.2 or 0.7 any time soon?

[Oleg Kalnichevski <ol...@apache.org>]

On Tue, 2011-01-18 at 17:46 +0100, Stefano Bagnara wrote:
> Il 17 gennaio 2011 17.43.37 UTC+1, Oleg Kalnichevski
> <ol...@apache.org> ha scritto:
> > Stefano et al
> >
> > Please review another round of API changes on the refactoring branch.
> > Here is the gist of most important changes
> >
> > * Most of the object model implementation logic moved from 'dom' to
> > 'message'. 'dom' consists mostly of pure interfaces now that do not
> > impose particular internal structures, a particular implementation or a
> > particular inheritance hierarchy.
> 
> I think we loose an opportunity to keep backward compatibility in
> future when we want to add methods to the 4 classes you transformed in
> interfaces and I don't see advantages in turn, but, on the other side
> I think we should feel free to break backward compatibility even in
> future versions so this is not a big drawback and I can live with the
> change (just hoping this won't be a blocker in future).
> 

Stefano

I understand the downside of having a pure interface based API. However,
you seem to be very keen to position mime4j not only as a utility for
parsing and building mime messages but also as an abstract Document
Object Model with different / alternative implementations. I just do not
see how one can honestly call the existing code in trunk an abstract
Object Model if the only aspect that is abstract is field parsing. We
should either drop the pretense at having an abstract DOM in mime4j and
revert to the state existed before your changes or actually make an
effort to define a truly abstract model even if that means more effort.

I am very fine with just reverting to the old state, by the way.

So, what shall I do?

> > * All dom element copy / parse logic moved from individual classes to a
> > utility class currently called MimeBuilder (I would prefer this class to
> > be called MessageBuilder but this name is already taken)
> 
> Maybe we should expose some of the MimeBuilder methods via the
> MessageBuilder "public" factory: the bigger limit of the current DOM
> api is that it doesn't expose methods to create a structured Message
> from scratch (it let you set a new Body but it doesn't give a way to
> create a Body). So the MimeBuilder methods could already be moved to
> MessageBuilderImpl. BTW I'm just loud thinking, just go ahead with
> your plan :-)
> 

I am going to get there shortly. I just prefer to make potentially
contentious changes in small, incremental steps, so that they are easier
to review and agree / disagree upon.

Oleg

Re: dom / message refactoring; was Re: 0.6.2 or 0.7 any time soon?

[Stefano Bagnara <io...@bago.org>]

Il 17 gennaio 2011 17.43.37 UTC+1, Oleg Kalnichevski
<ol...@apache.org> ha scritto:
> Stefano et al
>
> Please review another round of API changes on the refactoring branch.
> Here is the gist of most important changes
>
> * Most of the object model implementation logic moved from 'dom' to
> 'message'. 'dom' consists mostly of pure interfaces now that do not
> impose particular internal structures, a particular implementation or a
> particular inheritance hierarchy.

I think we loose an opportunity to keep backward compatibility in
future when we want to add methods to the 4 classes you transformed in
interfaces and I don't see advantages in turn, but, on the other side
I think we should feel free to break backward compatibility even in
future versions so this is not a big drawback and I can live with the
change (just hoping this won't be a blocker in future).

> * All dom element copy / parse logic moved from individual classes to a
> utility class currently called MimeBuilder (I would prefer this class to
> be called MessageBuilder but this name is already taken)

Maybe we should expose some of the MimeBuilder methods via the
MessageBuilder "public" factory: the bigger limit of the current DOM
api is that it doesn't expose methods to create a structured Message
from scratch (it let you set a new Body but it doesn't give a way to
create a Body). So the MimeBuilder methods could already be moved to
MessageBuilderImpl. BTW I'm just loud thinking, just go ahead with
your plan :-)

> Please let me know if you can live with these changes and whether it is
> ok to merge them down to trunk.

ok.

Stefano