[Twisted-Python] Re: supressess warnings

[Itamar Shtull-Trauring]

On Tue, 1 Apr 2003 16:48:01 -0500
Andrew Dalke <dalke at 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