Hi Imran,
Looks good. Thanks very much for spending the time on this.
I will integrate this week.
Paul.
On Mar 25, 2009, at 9:52 AM, Imran M Yousuf wrote:
> On Wed, Mar 25, 2009 at 11:31 AM, Imran M Yousuf
> <imran_at_smartitengineering.com> wrote:
>> On Tue, Mar 24, 2009 at 9:25 PM, Paul Sandoz <Paul.Sandoz_at_sun.com>
>> wrote:
>>>
>>> <snip />
>>> Patch one:
>>>
>>> Would it not be simpler to check if name == null and throw an
>>> IllegalArgumentException in *either* case of there being an
>>> existing content
>>> disposition value or not? rather than silently consuming the
>>> exception from
>>> the builder?
>>>
>>
>> I did not want to throw the exception, but I agree that I should have
>> done the same thing using name == null checking and will do that and
>> re-submit the patch and will also throw the exception if name ==
>> null.
>>
>
> Now, if name == null it will throw IlleagalArgumentException and the
> change reflects in the test cases as well.
>
>>>
>>> Patch two:
>>>
>>> I like the idea of FileDataBodyPart. We should have a
>>> corresponding "field"
>>> builder method for this.
>>>
>>
>> Hmm, I did not think of the builder, will add it to this patch as
>> well
>> and it will take 2 params, 'name' and 'file', what do you think?
>>
>
> I could find the usage of the builder and thus skipped it, if you can
> add some specification for it I will add them as another patch.
>
>>> Note that there is extensible support in SE 6 for guessing the
>>> media type
>>> from a file [1], and i think those using SE 6 should be able to
>>> leverage
>>> that. So even though we depend on SE 5 and above i am a bit
>>> reluctant to
>>> include such functionality as it is rather limited in the sense of
>>> extensibility of the media types and i would prefer not to
>>> duplicate the
>>> functionality in SE 6.
>>>
>>> Perhaps instead developers should be allowed to extend this class
>>> with the
>>> ability to guess the media type?
>>>
>>
>> I prefer the second option and would have the current mechanism as
>> the
>> default, will add a micro API for this in this patch. I prefer no
>> dependency of SE 6 libs and also the later option will provide
>> greater
>> extensibility.
>>
>
> Implemented the second option.
>
>>>
>>> Patch three & four
>>>
>>> Looks good!
>>>
>>
>> Cool!
>>
>> I will rewrite the patches tonight and will submit them again
>> tomorrow. Thank you for your detailed review.
>>
>
> All the patches are added to the issue as well as in this email.
> Please let me know if any other additions are necessary.
>
> Thank you,
>
> - Imran
>
>> Best regards,
>>
>> - Imran
>>
>>> Paul.
>>>
>>> [1]
>>> http://java.sun.com/javase/6/docs/api/javax/activation/MimetypesFileTypeMap.html
>>>
>>>
>>>> Thanks,
>>>>
>>>> Imran
>>>>
>>>>> Paul.
>>>>>
>>
>
>
>
> --
> Imran M Yousuf
> Entrepreneur & Software Engineer
> Smart IT Engineering
> Dhaka, Bangladesh
> Email: imran_at_smartitengineering.com
> Blog: http://imyousuf-tech.blogs.smartitengineering.com/
> Mobile: +880-1711402557
> <0001-Fix-content-disposition-construction-with-null-
> name.patch><0002-Add-convenience-form-body-part-extension-for-
> java.io.patch><0003-Fix-Date-to-String-conversion-in-Content-
> Disposition.patch><0004-Add-test-case-for-ContentDisposition-and-
> FormDataCon
> .patch
> >---------------------------------------------------------------------
> To unsubscribe, e-mail: users-unsubscribe_at_jersey.dev.java.net
> For additional commands, e-mail: users-help_at_jersey.dev.java.net