Re: [Twisted-web] fix web2 connection lost error handler
On Sun, 22 Jan 2006 05:27:18 +0100, Andrea Arcangeli <andrea@cpushare.com> wrote:
comments and patch here:
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?
On Sun, Jan 22, 2006 at 05:02:41AM -0500, glyph@divmod.com wrote:
Please send comments and patches to the bugtracker:
Done.
It's a basic courtesy, like using the proper format of diff. You can just
Format diff is one click away, "raw". You want me to use the web to file bugreports, so I thought you would be confortable with web reports too ;)
slow and stylistically wrong instead of fast and logically wrong. The appropriate API to use there is reason.check(error.ConnectionLost).
That's nicer, I'll fix that thanks!
- 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.
It's a design decision, I want twisted to never generate exceptions, unless there's a bug in twisted. An exception showing up in the log must be like a kernel oops, when it happens it must signal a bug. I considered logging a message but I thought it was not worth it, but OTOH given this is still code under development, you're probably right that it's a good idea (so if it happens too often we may want to review the code).
One procedural error for every line of code that you changed - *and* you
My code work right, SVN didn't, so don't blame on me for fixing bugs please. It's only a style error, not anything that could harm the runtime. But I agree in making it more eye candy.
participants (2)
-
Andrea Arcangeli -
glyph＠divmod.com