Launchpad itself

Merge lp:~jml/launchpad/warnings-for-unproxied into lp:launchpad

  1. warnings-for-unproxied
  2. Merge into devel
Proposed by Jonathan Lange
Status: Merged
Approved by: Aaron Bentley
Approved revision: no longer in the source branch.
Merged at revision: 11263
Proposed branch: lp:~jml/launchpad/warnings-for-unproxied
Merge into: lp:launchpad
Diff against target: 110 lines (+35/-14)
3 files modified
buildout-templates/bin/test.in (+5/-4)
lib/lp/scripts/utilities/warninghandler.py (+4/-2)
lib/lp/testing/factory.py (+26/-8)
To merge this branch: bzr merge lp:~jml/launchpad/warnings-for-unproxied
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Approve
Review via email: mp+30891@code.launchpad.net

Commit message

Use warnings instead of printing to stderr for unwrapped proxy methods.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'buildout-templates/bin/test.in'
2--- buildout-templates/bin/test.in 2010-07-20 04:46:46 +0000
3+++ buildout-templates/bin/test.in 2010-07-30 11:56:08 +0000
4@@ -160,10 +160,11 @@
5 re.escape('clear_request_started() called outside of a request'),
6 UserWarning
7 )
8-
9-# Any warnings not explicitly silenced are errors
10-warnings.filterwarnings('error', append=True)
11-
12+# Unicode warnings are always fatal
13+warnings.filterwarnings('error', category=UnicodeWarning)
14+
15+# shortlist() raises an error when it is misused.
16+warnings.filterwarnings('error', r'shortlist\(\)')
17
18 from canonical.ftests import pgsql
19 # If this is removed, make sure canonical.ftests.pgsql is updated
20
21=== modified file 'lib/lp/scripts/utilities/warninghandler.py'
22--- lib/lp/scripts/utilities/warninghandler.py 2009-06-25 04:06:00 +0000
23+++ lib/lp/scripts/utilities/warninghandler.py 2010-07-30 11:56:08 +0000
24@@ -150,10 +150,13 @@
25 other_warnings = []
26
27 old_show_warning = warnings.showwarning
28-def launchpad_showwarning(message, category, filename, lineno, file=None):
29+def launchpad_showwarning(message, category, filename, lineno, file=None,
30+ line=None):
31 if file is None:
32 file = sys.stderr
33 stream = StringIO.StringIO()
34+ # XXX: JonathanLange 2010-07-27: When Launchpad ceases supporting Python
35+ # 2.5, pass on the optional 'line' parameter.
36 old_show_warning(message, category, filename, lineno, stream)
37 warning_message = stream.getvalue()
38 important_info = find_important_info()
39@@ -201,7 +204,6 @@
40 print "General warnings."
41 for warninginfo in other_warnings:
42 print
43- print warninginfo.message,
44 print warninginfo
45
46 def report_warnings():
47
48=== modified file 'lib/lp/testing/factory.py'
49--- lib/lp/testing/factory.py 2010-07-30 03:55:05 +0000
50+++ lib/lp/testing/factory.py 2010-07-30 11:56:08 +0000
51@@ -35,6 +35,7 @@
52 from textwrap import dedent
53 from threading import local
54 from types import InstanceType
55+import warnings
56
57 import pytz
58
59@@ -2786,6 +2787,28 @@
60 return False
61
62
63+class UnproxiedFactoryMethodWarning(UserWarning):
64+ """Raised when someone calls an unproxied factory method."""
65+
66+ def __init__(self, method_name):
67+ super(UnproxiedFactoryMethodWarning, self).__init__(
68+ "PLEASE FIX: LaunchpadObjectFactory.%s returns an "
69+ "unproxied object." % (method_name,))
70+
71+
72+class UnreasonableRemoveSecurityProxyWarning(UserWarning):
73+ """Raised when there is an unreasonable call to removeSecurityProxy."""
74+
75+ # XXX: JonathanLange 2010-07-25: I have no idea what "unreasonable" means
76+ # in this context.
77+
78+ def __init__(self, obj):
79+ message = (
80+ "Called removeSecurityProxy(%r) without a check if this is "
81+ "reasonable." % obj)
82+ super(UnreasonableRemoveSecurityProxyWarning, self).__init__(message)
83+
84+
85 class LaunchpadObjectFactory:
86 """A wrapper around `BareLaunchpadObjectFactory`.
87
88@@ -2808,10 +2831,8 @@
89 def guarded_method(*args, **kw):
90 result = attr(*args, **kw)
91 if not is_security_proxied_or_harmless(result):
92- message = (
93- "PLEASE FIX: LaunchpadObjectFactory.%s returns an "
94- "unproxied object." % name)
95- print >>sys.stderr, message
96+ warnings.warn(
97+ UnproxiedFactoryMethodWarning(name), stacklevel=1)
98 return result
99 return guarded_method
100 else:
101@@ -2828,8 +2849,5 @@
102 This function should only be used in legacy tests which fail because
103 they expect unproxied objects.
104 """
105- print >>sys.stderr, (
106- "\nWarning: called removeSecurityProxy() for %r without a check if "
107- "this reasonable. Look for a call of "
108- "remove_security_proxy_and_shout_at_engineer(some_object)." % obj)
109+ warnings.warn(UnreasonableRemoveSecurityProxyWarning(obj), stacklevel=2)
110 return removeSecurityProxy(obj)