In message <[EMAIL PROTECTED]>, Jeffrey D. Brekke writes: >Applying the patch and cutting a RC1 gets a +1 from me. > >I'd say after Wed./RC1, leave the RC1 out for a week, then if no >issues show up do a 1.1 release?
I didn't get around to applying the changes suggested by the patch until today. After making the changes, I found myself writing a commit message that explained the reason for the changes and why they weren't applied to classes like FTPClient and NNTPClient. In the process of writing the message, I aborted the commit to revisit why we need these changes. The isConnected() and isOpen() checks only appear to be okay because the classes to which they apply perform only one action. However, imagine if we checked isConnected() in every method of FTPClient. I'd rather have a consistent policy toward which programming mistakes we protect against and which we don't. If the reason for implementing protective measure applies to all of the client classes, but the change itself is not suitable for all of the classes, I'd rather not implement it for any of the classes and delegate the responsibility to the API user. The reasoning for the null checks in close() made sense when explained, but after applying the changes they didn't make sense to me. First, the proposed change for SocketClient swallows exceptions we want reported to the programmer. So I changed the patch to keep the null checks, but rethrow the exception. However, the way close() is implemented originally, a socket or stream reference cannot ever be null if close() throws an exception (all of the null assignments are made after the close() calls). In other words, if you call close() twice in succession and the first call fails, the second call will throw an exception other than NullPointerException. If the first call succeeds then the second call will indeed result in the accessing of null references. However, programmers shouldn't be writing code that allows a close() or disconnect() to be called in a finally block after a successful call to close() or disconnect(). They should check in the finally block whether the client is connected or open with isConnected() or isOpen(). By applying the suggested changes, we encourage careless programming. Now, don't get me wrong, the Commons Net API is by no means a model of perfection and it could really use an overhaul because it makes dealing with exceptions messy (e.g., src/java/examples/ftp.java is evidence of this). So it's not like I'm saying we don't have things we need to do to make programming with Commons Net cleaner. In any case, I've talked my way back to being -1 on the changes. If I wasn't very clear in my explanation of why, let me know and I'll clarify. I suggest we move forward with a 1.1 RC1 vote as Jeffrey proposed and resolve any further debate about this patch after the 1.1 release. daniel --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
