james-mime4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefano Bagnara <apa...@bago.org>
Subject Re: [cycleclean] minor naming tweaks; was Re: [cycleclean] branch review and questions
Date Fri, 08 Jan 2010 18:43:16 GMT
2010/1/8 Oleg Kalnichevski <olegk@apache.org>:
> Stefano Bagnara wrote:
>>
>> 2010/1/8 Oleg Kalnichevski <olegk@apache.org>:
>>>
>>> With so many classes moved to different packages an iterative merge
>>> would just be too hard. I am +1 to merging the entire branch down to
>>> trunk. Remaining issues can be dealt with once the branch has been
>>> merged.
>>>
>>> Minor stuff:
>>>
>>> (1) I also would like to propose a few minor changes / renames. Ideally,
>>> I would like the 'steam' package to be fully usable out of the box. So,
>>> it would be good if DefaultBodyDescriptor was moved to 'steam' and
>>> renamed to BasicBodyDescriptor for consistency. I also think
>>> FullBodyDescriptor is a better name for MaximalBodyDescriptor
>>
>> DefaultBodyDescriptor would reintroduce the field parsing dependency
>> to the stream package.
>
> Unless I am missing something, I can't see any mime4j dependencies other
> than on 'util' and 'stream'
> ---
> import org.apache.james.mime4j.stream.BodyDescriptor;
> import org.apache.james.mime4j.stream.MutableBodyDescriptor;
> import org.apache.james.mime4j.stream.RawField;
> import org.apache.james.mime4j.util.MimeUtil;
> ---
> I do not see why this class cannot be moved to 'stream' without introducing
> cyclic dependencies.

I'll check it. In my mind it was dependin on field parsing, but I
remember I moved there the "getHeaderParams" method, so it does its
own parsing. In my mind the right solution was to move also that
method to the field parsing packages and so also this class would
depend on the field parsing, btw maybe you are right and we can move
the default body descriptor there, by now.

> So, unless we say that we don't care about
>>
>> separation between stream parser and field parsing, there is no way to
>> use stream alone.
>
> I just do not want to end up in a situation when 'stream' package simply
> cannot be used without 'parser' stuff thus rendering the whole idea of
> separating the two pointless.

It is a detail, but I don't agree with this reasoning. packages help
defining the project structure and the component dependencies.
Whenever it is possible to put evidence on the dependencies of a group
of classes and their concerns it is a good practice to do that. With
your reasoning abstract packages (interfaces and abstract classes
only) wouldn't exists because they cannot be used alone!

BTW let's move the default body descriptor there, by now, and we'll
see where the future bring us.

>> Thank you for reviewing, and let me know if you prefer the older
>> packaging or the newer one (cycleclean vs
>> cycleclean-pre-packagerenames)
>> Stefano
>
> I am fine with the new package names, hence my +1 to merge.

OK,
Stefano

Mime
View raw message