launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

Jonathan Lange has proposed merging lp:~jml/launchpad/warnings-for-unproxied into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This branch changes the warnings printed directly by Abel's wrapper to use Python's warning system directly.

IMPORTANT: This branch also changes our long-standing policy of raising errors in the test suite whenever there is a non-silenced warning. I can think of no good reason for this policy.

Otherwise, the changes are quite simple:
  - add two new warning classes
  - use these warnings instead of printing to stderr
    - stacklevel=1 for the factory wrapper guarantees only one warning per bad factory method
    - stacklevel=2 for the "and shout at engineer" function guarantees only one warning per caller
  - change the warning summarizer to only print one copy of each error

I've added a XXX because the criteria for remove_security_proxy_and_shout_at_engineer are unclear to me.
-- 
https://code.launchpad.net/~jml/launchpad/warnings-for-unproxied/+merge/30891
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/warnings-for-unproxied into lp:launchpad/devel.

=== modified file 'buildout-templates/bin/test.in'
--- buildout-templates/bin/test.in	2010-07-20 04:46:46 +0000
+++ buildout-templates/bin/test.in	2010-07-25 13:28:50 +0000
@@ -161,9 +161,6 @@
     UserWarning
     )
 
-# Any warnings not explicitly silenced are errors
-warnings.filterwarnings('error', append=True)
-
 
 from canonical.ftests import pgsql
 # If this is removed, make sure canonical.ftests.pgsql is updated

=== modified file 'lib/lp/scripts/utilities/warninghandler.py'
--- lib/lp/scripts/utilities/warninghandler.py	2009-06-25 04:06:00 +0000
+++ lib/lp/scripts/utilities/warninghandler.py	2010-07-25 13:28:50 +0000
@@ -150,11 +150,12 @@
 other_warnings = []
 
 old_show_warning = warnings.showwarning
-def launchpad_showwarning(message, category, filename, lineno, file=None):
+def launchpad_showwarning(message, category, filename, lineno, file=None,
+                          line=None):
     if file is None:
         file = sys.stderr
     stream = StringIO.StringIO()
-    old_show_warning(message, category, filename, lineno, stream)
+    old_show_warning(message, category, filename, lineno, stream, line=line)
     warning_message = stream.getvalue()
     important_info = find_important_info()
 
@@ -201,7 +202,6 @@
         print "General warnings."
         for warninginfo in other_warnings:
             print
-            print warninginfo.message,
             print warninginfo
 
 def report_warnings():

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-07-23 13:32:10 +0000
+++ lib/lp/testing/factory.py	2010-07-25 13:28:50 +0000
@@ -35,6 +35,7 @@
 from textwrap import dedent
 from threading import local
 from types import InstanceType
+import warnings
 
 import pytz
 
@@ -2757,6 +2758,28 @@
     return False
 
 
+class UnproxiedFactoryMethodWarning(UserWarning):
+    """Raised when someone calls an unproxied factory method."""
+
+    def __init__(self, method_name):
+        super(UnproxiedFactoryMethodWarning, self).__init__(
+            "PLEASE FIX: LaunchpadObjectFactory.%s returns an "
+            "unproxied object." % (method_name,))
+
+
+class UnreasonableRemoveSecurityProxyWarning(UserWarning):
+    """Raised when there is an unreasonable call to removeSecurityProxy."""
+
+    # XXX: JonathanLange 2010-07-25: I have no idea what "unreasonable" means
+    # in this context.
+
+    def __init__(self, obj):
+        message = (
+            "Called removeSecurityProxy(%r) without a check if this is "
+            "reasonable." % obj)
+        super(UnreasonableRemoveSecurityProxyWarning, self).__init__(message)
+
+
 class LaunchpadObjectFactory:
     """A wrapper around `BareLaunchpadObjectFactory`.
 
@@ -2778,10 +2801,8 @@
             def guarded_method(*args, **kw):
                 result = attr(*args, **kw)
                 if not is_security_proxied_or_harmless(result):
-                    message = (
-                        "PLEASE FIX: LaunchpadObjectFactory.%s returns an "
-                        "unproxied object." % name)
-                    print >>sys.stderr, message
+                    warnings.warn(
+                        UnproxiedFactoryMethodWarning(name), stacklevel=1)
                 return result
             return guarded_method
         else:
@@ -2798,8 +2819,5 @@
     This function should only be used in legacy tests which fail because
     they expect unproxied objects.
     """
-    print >>sys.stderr, (
-        "\nWarning: called removeSecurityProxy() for %r without a check if "
-        "this reasonable. Look for a call of "
-        "remove_security_proxy_and_shout_at_engineer(some_object)." % obj)
+    warnings.warn(UnreasonableRemoveSecurityProxyWarning(obj), stacklevel=2)
     return removeSecurityProxy(obj)