On Tue, 1 Apr 2003 16:48:01 -0500 Andrew Dalke <dalke@dalkescientific.com> wrote:
to actually use the log. It isn't obvious anywhere that that import will make a system level change. - what if I've changed warnings.showwarning in my own code (or in some other 3rd party package). Where's the documentation which states that there will be a problem?
- It's pervasive. That is, if I import twisted.lore (perhaps because I want it to use it for my own documentation system) it's still the case that warnings.showwarning gets replaced. There should be no reason for that.
I agree that this is bad. If you complain enough maybe glyph will listen to reason.
Are there any other system-level functions or settings which get tweaked by Twisted?
Not that I can recall.
There are only 4 places which use that, outside of some of the test code. Is it really that useful given how non-obvious it is?
No. We need to have thread-safe logging.
Notice that log.py's Log class, which has a 'synchronized' attribute, is not so initialized. This suggests there's either a bug (this class needs to be synchronized) or that that attribute is unneeded.
Indeed.
I still don't like that it goes into effect without me doing anything other than an import.
Yes. Its glyph's fault, again.
Looking only at the error message, it means that: - the option parsing code does automatic prefix expansion, so that if the option is '--pop3' and the argument is '--pop' and no other option starts with '--pop' then it's elided to '--pop3'. ... - and no one has run those tests in a while, probably not since 10-Feb-03 when moshez added the following option.
["pop3s", "S", 0, "Port to start the POP3-over-SSL server on (0 to disable)."],
Yes, they only get run during releases. Jean-Paul fixed the --pop issue in CVS I think. Some of the failed tests look like timing issues, yes.
------- Traceback (most recent call last): File "/Users/dalke/cvses/Twisted/twisted/trial/unittest.py", line
165, in runOneTest method(testCase) File "/Users/dalke/cvses/Twisted/twisted/test/test_process.py", line 140, in testEcho self.assertEquals(len(p.buffer), len(p.s * 10)) AttributeError: EchoProtocol instance has no attribute 'buffer'
Gar. I wonder why this isn't showing on the Max OS X buildbot.
Trying to track that down, I see that twisted/internet/interfaces.py has mention of @see: C{twisted.protocols.protocol.ProcessProtocol}
Typo - fixed in CVS. Any bugs we can reproduce and are easily fixable will be fixed by next release, e.g. the dom one.
As mentioned, I don't like how system settings are changed on an import, much less without documentation which says this will happen.
We should have logging docs soon.
If everything server related should be done in the context of twistd, then that warnings.showwarning replacement should only be done when twistd is used, which is why the patch I sent only made the switch when startLogging was called.
Again, I agree, bug glyph.
The solution I see now, which is that showwarnings calls the errlog instead of the msg log, still isn't the right behaviour, IMO, if only because you are changing the format of the error messages. At the very least you should call warnings.formatwarning instead of building your own warning message.
The idea was to have more informative messages. Can you put in something comparing the two formats?
def showwarning(message, category, filename, lineno, file=None): m = warnings.formatwarning(message, category, filename, lineno) if file is None: err(m) else: file.write(m)
Done, at least partially.
Finally, I noticed that other part of my patch were not accepted. For example, the use of os.linesep is incorrect. The log file is opened in text mode, so "\n" will be converted to the appropriate byte representation for the given platform. In other words,
logerr.write(str(stuff)+os.linesep) should be logerr.write(str(stuff)+"\n")
I also did some things to clean up the code, like getting rid of unneeded module imports (like string) and replacing
I noticed none of these were accepted.
Not really, I just didn't get around to reading the patch. I did some of these in CVS.
What should I do to make these sorts of changes more acceptable?
They were not bad, it seems, we just don't have time to deal with all of them... I have a huge queue waiting for me :( We do appreciate your efforts.
After all, the point of a good unit test suite is to make it easy for people to refactor and clean up existing code. All the tests passed identically before and after my changes.
Yes, but behaviour was changed I think? Possibly in a better way, but changing behaviour takes more mental work on our part. -- Itamar Shtull-Trauring http://itamarst.org/ http://www.zoteca.com -- Python & Twisted consulting