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
=== 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-30 11:56:08 +0000
@@ -160,10 +160,11 @@
160 re.escape('clear_request_started() called outside of a request'),160 re.escape('clear_request_started() called outside of a request'),
161 UserWarning161 UserWarning
162 )162 )
163163# Unicode warnings are always fatal
164# Any warnings not explicitly silenced are errors164warnings.filterwarnings('error', category=UnicodeWarning)
165warnings.filterwarnings('error', append=True)165
166166# shortlist() raises an error when it is misused.
167warnings.filterwarnings('error', r'shortlist\(\)')
167168
168from canonical.ftests import pgsql169from canonical.ftests import pgsql
169# If this is removed, make sure canonical.ftests.pgsql is updated170# If this is removed, make sure canonical.ftests.pgsql is updated
170171
=== 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-30 11:56:08 +0000
@@ -150,10 +150,13 @@
150other_warnings = []150other_warnings = []
151151
152old_show_warning = warnings.showwarning152old_show_warning = warnings.showwarning
153def launchpad_showwarning(message, category, filename, lineno, file=None):153def launchpad_showwarning(message, category, filename, lineno, file=None,
154 line=None):
154 if file is None:155 if file is None:
155 file = sys.stderr156 file = sys.stderr
156 stream = StringIO.StringIO()157 stream = StringIO.StringIO()
158 # XXX: JonathanLange 2010-07-27: When Launchpad ceases supporting Python
159 # 2.5, pass on the optional 'line' parameter.
157 old_show_warning(message, category, filename, lineno, stream)160 old_show_warning(message, category, filename, lineno, stream)
158 warning_message = stream.getvalue()161 warning_message = stream.getvalue()
159 important_info = find_important_info()162 important_info = find_important_info()
@@ -201,7 +204,6 @@
201 print "General warnings."204 print "General warnings."
202 for warninginfo in other_warnings:205 for warninginfo in other_warnings:
203 print206 print
204 print warninginfo.message,
205 print warninginfo207 print warninginfo
206208
207def report_warnings():209def report_warnings():
208210
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-07-30 03:55:05 +0000
+++ lib/lp/testing/factory.py 2010-07-30 11:56:08 +0000
@@ -35,6 +35,7 @@
35from textwrap import dedent35from textwrap import dedent
36from threading import local36from threading import local
37from types import InstanceType37from types import InstanceType
38import warnings
3839
39import pytz40import pytz
4041
@@ -2786,6 +2787,28 @@
2786 return False2787 return False
27872788
27882789
2790class UnproxiedFactoryMethodWarning(UserWarning):
2791 """Raised when someone calls an unproxied factory method."""
2792
2793 def __init__(self, method_name):
2794 super(UnproxiedFactoryMethodWarning, self).__init__(
2795 "PLEASE FIX: LaunchpadObjectFactory.%s returns an "
2796 "unproxied object." % (method_name,))
2797
2798
2799class UnreasonableRemoveSecurityProxyWarning(UserWarning):
2800 """Raised when there is an unreasonable call to removeSecurityProxy."""
2801
2802 # XXX: JonathanLange 2010-07-25: I have no idea what "unreasonable" means
2803 # in this context.
2804
2805 def __init__(self, obj):
2806 message = (
2807 "Called removeSecurityProxy(%r) without a check if this is "
2808 "reasonable." % obj)
2809 super(UnreasonableRemoveSecurityProxyWarning, self).__init__(message)
2810
2811
2789class LaunchpadObjectFactory:2812class LaunchpadObjectFactory:
2790 """A wrapper around `BareLaunchpadObjectFactory`.2813 """A wrapper around `BareLaunchpadObjectFactory`.
27912814
@@ -2808,10 +2831,8 @@
2808 def guarded_method(*args, **kw):2831 def guarded_method(*args, **kw):
2809 result = attr(*args, **kw)2832 result = attr(*args, **kw)
2810 if not is_security_proxied_or_harmless(result):2833 if not is_security_proxied_or_harmless(result):
2811 message = (2834 warnings.warn(
2812 "PLEASE FIX: LaunchpadObjectFactory.%s returns an "2835 UnproxiedFactoryMethodWarning(name), stacklevel=1)
2813 "unproxied object." % name)
2814 print >>sys.stderr, message
2815 return result2836 return result
2816 return guarded_method2837 return guarded_method
2817 else:2838 else:
@@ -2828,8 +2849,5 @@
2828 This function should only be used in legacy tests which fail because2849 This function should only be used in legacy tests which fail because
2829 they expect unproxied objects.2850 they expect unproxied objects.
2830 """2851 """
2831 print >>sys.stderr, (2852 warnings.warn(UnreasonableRemoveSecurityProxyWarning(obj), stacklevel=2)
2832 "\nWarning: called removeSecurityProxy() for %r without a check if "
2833 "this reasonable. Look for a call of "
2834 "remove_security_proxy_and_shout_at_engineer(some_object)." % obj)
2835 return removeSecurityProxy(obj)2853 return removeSecurityProxy(obj)