You are viewing a plain text version of this content. The canonical link for it is here.
Posted to mime4j-dev@james.apache.org by Ioan Eugen Stan <st...@gmail.com> on 2012/06/16 16:25:38 UTC
mbox-iterator in mime4j - code review
Hello,
I've completed integrating mbox-iterator[1] into mime4j. I'm waiting 3
days for a code review before I do a push. The changes to mime4j are
in this change-set on github:
https://github.com/ieugen/james-mime4j/commit/8619b28297d8762913754fcee8ceb98caad5d218
I added an example of how you can use it with mime4j to parse mbox files.
I also bumped junit to 4.10.
Cheers,
[1] https://github.com/ieugen/mbox-iterator
--
Ioan Eugen Stan / http://axemblr.com / Tools for Clouds
Re: mbox-iterator in mime4j - code review
Posted by Ioan Eugen Stan <st...@gmail.com>.
I've committed my changes, without doing a git rebase first.
2012/6/19 Ioan Eugen Stan <st...@gmail.com>:
> HI Oleg,
>
> Thanks for the review, please read my response inline.
>
> 2012/6/18 Oleg Kalnichevski <ol...@apache.org>:
>> On Sat, 2012-06-16 at 17:25 +0300, Ioan Eugen Stan wrote:
>>> Hello,
>>>
>>> I've completed integrating mbox-iterator[1] into mime4j. I'm waiting 3
>>> days for a code review before I do a push. The changes to mime4j are
>>> in this change-set on github:
>>>
>>> https://github.com/ieugen/james-mime4j/commit/8619b28297d8762913754fcee8ceb98caad5d218
>>>
>>> I added an example of how you can use it with mime4j to parse mbox files.
>>>
>>> I also bumped junit to 4.10.
>>>
>>> Cheers,
>>>
>>> [1] https://github.com/ieugen/mbox-iterator
>>
>> Ioan Eugen
>>
>> I took a cursory look at the changeset and it looked quite reasonable to
>> me. I would also advocate getting rid of dependency on Guava. Mime4j is
>> a low level library and ideally should not impose additional
>> dependencies on its consumers unless absolutely necessary.
>
> Yeah, I ill remove Guava dependency. It's not really needed.
>
>> You cannot just push your changeset to the original mime4j repository on
>> GitHub as it is a read only copy of the official SVN repository. You
>> will have to raise a JIRA issue for your contribution and attach the
>> patch to it. I an willing to commit the patch as is even with Guava
>> dependency, if no one vetoes it.
>
> I'm a James committer. Thanks for the offer. I know about git repo
> being read-only, I wanted to test git svn. I will commit after I make
> the changes you and Eric suggested.
>
>> Oleg
>>
>
> Cheers,
>
> --
> Ioan Eugen Stan / http://axemblr.com / Tools for Clouds
--
Ioan Eugen Stan / http://axemblr.com / Tools for Clouds
Re: mbox-iterator in mime4j - code review
Posted by Ioan Eugen Stan <st...@gmail.com>.
HI Oleg,
Thanks for the review, please read my response inline.
2012/6/18 Oleg Kalnichevski <ol...@apache.org>:
> On Sat, 2012-06-16 at 17:25 +0300, Ioan Eugen Stan wrote:
>> Hello,
>>
>> I've completed integrating mbox-iterator[1] into mime4j. I'm waiting 3
>> days for a code review before I do a push. The changes to mime4j are
>> in this change-set on github:
>>
>> https://github.com/ieugen/james-mime4j/commit/8619b28297d8762913754fcee8ceb98caad5d218
>>
>> I added an example of how you can use it with mime4j to parse mbox files.
>>
>> I also bumped junit to 4.10.
>>
>> Cheers,
>>
>> [1] https://github.com/ieugen/mbox-iterator
>
> Ioan Eugen
>
> I took a cursory look at the changeset and it looked quite reasonable to
> me. I would also advocate getting rid of dependency on Guava. Mime4j is
> a low level library and ideally should not impose additional
> dependencies on its consumers unless absolutely necessary.
Yeah, I ill remove Guava dependency. It's not really needed.
> You cannot just push your changeset to the original mime4j repository on
> GitHub as it is a read only copy of the official SVN repository. You
> will have to raise a JIRA issue for your contribution and attach the
> patch to it. I an willing to commit the patch as is even with Guava
> dependency, if no one vetoes it.
I'm a James committer. Thanks for the offer. I know about git repo
being read-only, I wanted to test git svn. I will commit after I make
the changes you and Eric suggested.
> Oleg
>
Cheers,
--
Ioan Eugen Stan / http://axemblr.com / Tools for Clouds
Re: mbox-iterator in mime4j - code review
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Sat, 2012-06-16 at 17:25 +0300, Ioan Eugen Stan wrote:
> Hello,
>
> I've completed integrating mbox-iterator[1] into mime4j. I'm waiting 3
> days for a code review before I do a push. The changes to mime4j are
> in this change-set on github:
>
> https://github.com/ieugen/james-mime4j/commit/8619b28297d8762913754fcee8ceb98caad5d218
>
> I added an example of how you can use it with mime4j to parse mbox files.
>
> I also bumped junit to 4.10.
>
> Cheers,
>
> [1] https://github.com/ieugen/mbox-iterator
Ioan Eugen
I took a cursory look at the changeset and it looked quite reasonable to
me. I would also advocate getting rid of dependency on Guava. Mime4j is
a low level library and ideally should not impose additional
dependencies on its consumers unless absolutely necessary.
You cannot just push your changeset to the original mime4j repository on
GitHub as it is a read only copy of the official SVN repository. You
will have to raise a JIRA issue for your contribution and attach the
patch to it. I an willing to commit the patch as is even with Guava
dependency, if no one vetoes it.
Oleg
Re: mbox-iterator in mime4j - code review
Posted by Eric Charles <er...@apache.org>.
Hi Ioan,
Patch looks good to me, except that I wonder why you to introduce Guava
dependency:
- The Preconditions could be replace by a simple test.
- The InputStream asInputStreamUTF8Encoded() {return new
ByteBufferInputStream(Charsets.UTF_8.encode(messageBuffer));} is not
used in the project.
- The Charset charset = Charsets.UTF_8; could be replace with
Charset.forName("UTF_8")
It compiles/tests fine on jdk6 (not on my jdk7 which complains
@Override, but that's a bug on my env).
Thx, Eric
PS: For those willing to test from ioan' repo, don't forget 'git
checkout -b master origin/master' after 'git clone
https://github.com/ieugen/james-mime4j.git', svn's trunk in git's master).
On 06/16/2012 04:25 PM, Ioan Eugen Stan wrote:
> Hello,
>
> I've completed integrating mbox-iterator[1] into mime4j. I'm waiting 3
> days for a code review before I do a push. The changes to mime4j are
> in this change-set on github:
>
> https://github.com/ieugen/james-mime4j/commit/8619b28297d8762913754fcee8ceb98caad5d218
>
> I added an example of how you can use it with mime4j to parse mbox files.
>
> I also bumped junit to 4.10.
>
> Cheers,
>
> [1] https://github.com/ieugen/mbox-iterator
--
eric | http://about.echarles.net | @echarles