cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Geoff Howard <>
Subject Re: before we release - multipart uploads
Date Thu, 12 Feb 2004 12:11:32 GMT
Sylvain Wallez wrote:

> Geoff Howard wrote:
>> I dug into upload tonight to help some confusion on the users list and 
>> noticed some new things which were introduced since 2.1.3 (in nov. by 
>> Sylvain to support woody).  I'd like to propose some tweaks which seem 
>> wise enough to me to get in quickly or it will be too late.


>> 2) The method dispose() was added by having Part implement Disposable. 
>> I like the method, but didn't like implementing a lifecycle method on 
>> what is otherwise not a component.  I think dispose (or another name 
>> if the overlapping name is deemed too confusing) should just be added 
>> to the abstract Part without Disposable.
> I personally don't see any problem with using this interface with 
> non-components, rather the contrary. Disposable clearly identifies that 
> some cleanup is required on the object, and everybody knows and 
> understands its semantics: do some cleanup at the end of the object's 
> life and never use it again. Sure, we can have dispose() without 
> implementing the interface, but doing it so IMO gives better visibility 
> to this important contract.

Ok.  Curious - is this the only case we have of a lifecycle interface 
applied to a non-component?

>> 3) dispose() is public - but it could be protected, or even package 
>> private.  I can't think of a reason why code outside this limited 
>> scope (the cleanup method above) should be calling dispose() directly, 
>> unless Woody has a use for it that I haven't found.
> dispose() *needs* to be public, as Woody's upload widget calls it. This 
> widget also calls setDisposeWithRequest(false).

Interesting - I couldn't find where woody was using dispose().  Why 
would it need to dispose instead of setDisposeWithRequest()?

>> 3) A public accessor is created called boolean disposeWithRequest(). 
>> This seems better called isDisposeWithRequest() and I don't see why it 
>> needs to be public.  It's intended use is in the cleanup method above, 
>> unless woody has a use for it that I haven't found.
> I agree with your naming concern, and you're right in saying it doesn't 
> need formally need to be public, as it's used only by MultipartRequest.
>> Sorry I just noticed these things - I glanced at the cvs mail at the 
>> time it went by but didn't notice the issues above until I dug through 
>> tonight.
> With the above comments, it seems to me the only problem is the 
> disposeWithRequest() acessor. I guess it's too late now to change it 
> (Carsten said "don't commit"), but this should not hurt much.

I think you're right that it's too late.  The problem is that the 
misnamed accessor sounds like it performs an action and so may be used 
by people mistakenly.  Would you be in favor of deprecating it right 
after release and creating a new method with the new name?  Normally I 
am not so focused on naming issues, but this is a part of our API that 
will be touched by a lot of users who already seem to have a hard time 
with uploads and I'm anticipating a fresh round of questions...


View raw message