Ticket #6087 regression closed fixed

Opened 19 months ago

Last modified 18 months ago

Restore (and test more reliabley) twisted.python.log's capture of warnings

Reported by: itamar Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: exarkun, ralphm Branch: branches/warnings-6087
(diff, github, buildbot, log)
Author: itamarst Launchpad Bug:

Description

Warnings are supposed to end up in the Twisted log once logging is initialized, but #5885 broke that functionality. It should be fixed (reverting #5885 would be somewhat painful at this point) and the tests improved so this doesn't happen again.

Change History

1

Changed 19 months ago by itamarst

  • branch set to branches/warnings-6087
  • branch_author set to itamarst

(In [36022]) Branching to 'warnings-6087'

2

Changed 19 months ago by itamar

  • keywords review added
  • owner itamar deleted
  • cc exarkun, ralphm added

I've fixed it, I think. Note that this involves a minor change in the way we do things; when we write a warning to a file in LogPublisher.showwarning, we use the *original* warnings.showwarning (or at least, what was there at import time). Previously we used the version captured by startLoggingWithObserver, which caused problems using LogPublisher.showwarning before logging had been initialized. It is vaguely possible this will break some obscure code somewhere that overrides showwarning sufficiently to actually write to files differently, and does the override in between importing twisted.python.log and starting twisted logging. I am inclined not to care, but I could come up with a different solution.

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/warnings-6087

3

Changed 19 months ago by itamar

  • type changed from defect to regression

4

Changed 18 months ago by exarkun

  • owner set to itamar
  • keywords review removed

Thanks.

  1. in test_publisherWarningInitialization
    1. the builtin setattr is preferable to using warnings.__setattr__.
  2. twisted/python/log.py
    1. As discussed on IRC, the showwarning APIs here aren't intended to be used by applications. I think we should move towards deprecating them and not worry about all the super obscure edge cases in their behavior (related to what order events happen in, etc).

Not sure many more review comments make sense, given that hopefully the last point above means things will change significantly and for the better.

5

Changed 18 months ago by itamar

  • keywords review added
  • owner changed from itamar to exarkun

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/warnings-6087 has started.

I implemented a simpler "fix"; we're back to the previous behavior from before the Python 3 changes, I believe, with some tests skipped or munged until they work.

6

Changed 18 months ago by exarkun

  • keywords review removed
  • owner changed from exarkun to itamar

Okay. Please merge.

7

Changed 18 months ago by itamarst

  • status changed from new to closed
  • resolution set to fixed

(In [36349]) Merge warnings-6087.

Author: itamar Review: exarkun Fixes: #6087

The Python 3 port of logging mistakenly broke warning integration; this fixes it. It's a .misc because from point of view of upgrading user nothing will have changed.

Note: See TracTickets for help on using tickets.