[Twisted-web] fix web2 connection lost error handler

[glyph at divmod.com]

On Sun, 22 Jan 2006 05:27:18 +0100, Andrea Arcangeli <andrea at cpushare.com> wrote:
>comments and patch here:
>
>http://www.cpushare.com/hg/Twisted/?cs=a924218612ef

Please send comments and patches to the bugtracker:

    http://twistedmatrix.com/bugs/

It's a basic courtesy, like using the proper format of diff.  You can just as easily link to external repositories or files from there as from the mailing list.

I skimmed the patch since it was so short, although I do wish it had been in the tracker.

 - You're disabling logging which someone else may have been depending upon in the same revision as you're fixing an arguably legitimate bug.  An exception was being trapped incorrectly.  Maybe you want to log a message rather than an error in the event of disconnects?  Maybe it should be an option?  At any rate, not separating the issues makes it harder to include the patch.

 - The aforementioned 'legitimate bugfix' is still wrong, except now it's slow and stylistically wrong instead of fast and logically wrong.  The appropriate API to use there is reason.check(error.ConnectionLost).  (Actually, I personally think it should be reason.trap(error.ConnectionLost), but then you'd depend on finalizing-deferred exception reporting, and it looks like someone was trying to avoid that...?)

 - You didn't really explain what the problem was, besides log noise.  What is the expected behavior?  Why is it expected?  What is the incorrect observed behavior?  You're randomly changing things around to suit your personal preferences (Twisted does not include an SMS log exception reporter, so I'm sure that wasn't bothering anyone else), and while that's fine for you, to be included in a release other people will use, it should have some justification.

One procedural error for every line of code that you changed - *and* you didn't write tests, not because you didn't have time yet, but because you refuse to.  Do you really wonder why your patches don't get included?