Merge lp:~spiv/bzr/pretty-decorator-vs-string-identity-718569 into lp:bzr

Proposed by Andrew Bennetts
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 5664
Proposed branch: lp:~spiv/bzr/pretty-decorator-vs-string-identity-718569
Merge into: lp:bzr
Diff against target: 153 lines (+47/-9)
3 files modified
bzrlib/decorators.py (+22/-8)
bzrlib/tests/test_decorators.py (+21/-1)
doc/en/release-notes/bzr-2.4.txt (+4/-0)
To merge this branch: bzr merge lp:~spiv/bzr/pretty-decorator-vs-string-identity-718569
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Review via email: mp+49614@code.launchpad.net

Commit message

Preserve identity of default values in the pretty decorators. (#718569)

Description of the change

This fixes bug 718569. Instead of relying on the repr() of default parameter values preserving the semantics of those values (which isn't the case of at least symbol_versioning.DEPRECATED_PARAMETER), it runs the exec statement with a scope that provides the original objects, and uses those instead.

All the existing tests for prettiness of the "pretty" decorators pass, so presumably this doesn't uglify them too much :) Correctness trumps prettiness anyway. I added new tests to catch this problem.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

Sounds reasonable and tested. A bit scary in retrospect, nice catch !

review: Approve
Revision history for this message
Andrew Bennetts (spiv) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/decorators.py'
--- bzrlib/decorators.py 2010-02-17 17:11:16 +0000
+++ bzrlib/decorators.py 2011-02-14 12:09:20 +0000
@@ -30,14 +30,16 @@
30def _get_parameters(func):30def _get_parameters(func):
31 """Recreate the parameters for a function using introspection.31 """Recreate the parameters for a function using introspection.
3232
33 :return: (function_params, calling_params)33 :return: (function_params, calling_params, default_values)
34 function_params: is a string representing the parameters of the34 function_params: is a string representing the parameters of the
35 function. (such as "a, b, c=None, d=1")35 function. (such as "a, b, c=None, d=1")
36 This is used in the function declaration.36 This is used in the function declaration.
37 calling_params: is another string representing how you would call the37 calling_params: is another string representing how you would call the
38 function with the correct parameters. (such as "a, b, c=c, d=d")38 function with the correct parameters. (such as "a, b, c=c, d=d")
39 Assuming you sued function_params in the function declaration, this39 Assuming you used function_params in the function declaration, this
40 is the parameters to put in the function call.40 is the parameters to put in the function call.
41 default_values_block: a dict with the default values to be passed as
42 the scope for the 'exec' statement.
4143
42 For example:44 For example:
4345
@@ -49,9 +51,15 @@
49 # it globally.51 # it globally.
50 import inspect52 import inspect
51 args, varargs, varkw, defaults = inspect.getargspec(func)53 args, varargs, varkw, defaults = inspect.getargspec(func)
54 defaults_dict = {}
55 def formatvalue(value):
56 default_name = '__default_%d' % len(defaults_dict)
57 defaults_dict[default_name] = value
58 return '=' + default_name
52 formatted = inspect.formatargspec(args, varargs=varargs,59 formatted = inspect.formatargspec(args, varargs=varargs,
53 varkw=varkw,60 varkw=varkw,
54 defaults=defaults)61 defaults=defaults,
62 formatvalue=formatvalue)
55 if defaults is None:63 if defaults is None:
56 args_passed = args64 args_passed = args
57 else:65 else:
@@ -65,7 +73,7 @@
65 args_passed.append('**' + varkw)73 args_passed.append('**' + varkw)
66 args_passed = ', '.join(args_passed)74 args_passed = ', '.join(args_passed)
6775
68 return formatted[1:-1], args_passed76 return formatted[1:-1], args_passed, defaults_dict
6977
7078
71def _pretty_needs_read_lock(unbound):79def _pretty_needs_read_lock(unbound):
@@ -107,14 +115,17 @@
107 return result115 return result
108read_locked = %(name)s_read_locked116read_locked = %(name)s_read_locked
109"""117"""
110 params, passed_params = _get_parameters(unbound)118 params, passed_params, defaults_dict = _get_parameters(unbound)
111 variables = {'name':unbound.__name__,119 variables = {'name':unbound.__name__,
112 'params':params,120 'params':params,
113 'passed_params':passed_params,121 'passed_params':passed_params,
114 }122 }
115 func_def = template % variables123 func_def = template % variables
116124
117 exec func_def in locals()125 scope = dict(defaults_dict)
126 scope['unbound'] = unbound
127 exec func_def in scope
128 read_locked = scope['read_locked']
118129
119 read_locked.__doc__ = unbound.__doc__130 read_locked.__doc__ = unbound.__doc__
120 read_locked.__name__ = unbound.__name__131 read_locked.__name__ = unbound.__name__
@@ -172,14 +183,17 @@
172 return result183 return result
173write_locked = %(name)s_write_locked184write_locked = %(name)s_write_locked
174"""185"""
175 params, passed_params = _get_parameters(unbound)186 params, passed_params, defaults_dict = _get_parameters(unbound)
176 variables = {'name':unbound.__name__,187 variables = {'name':unbound.__name__,
177 'params':params,188 'params':params,
178 'passed_params':passed_params,189 'passed_params':passed_params,
179 }190 }
180 func_def = template % variables191 func_def = template % variables
181192
182 exec func_def in locals()193 scope = dict(defaults_dict)
194 scope['unbound'] = unbound
195 exec func_def in scope
196 write_locked = scope['write_locked']
183197
184 write_locked.__doc__ = unbound.__doc__198 write_locked.__doc__ = unbound.__doc__
185 write_locked.__name__ = unbound.__name__199 write_locked.__name__ = unbound.__name__
186200
=== modified file 'bzrlib/tests/test_decorators.py'
--- bzrlib/tests/test_decorators.py 2010-04-23 08:51:52 +0000
+++ bzrlib/tests/test_decorators.py 2011-02-14 12:09:20 +0000
@@ -27,11 +27,13 @@
27 pass27 pass
2828
2929
30def create_decorator_sample(style, unlock_error=None):30def create_decorator_sample(style, unlock_error=None, meth=None):
31 """Create a DecoratorSample object, using specific lock operators.31 """Create a DecoratorSample object, using specific lock operators.
3232
33 :param style: The type of lock decorators to use (fast/pretty/None)33 :param style: The type of lock decorators to use (fast/pretty/None)
34 :param unlock_error: If specified, an error to raise from unlock.34 :param unlock_error: If specified, an error to raise from unlock.
35 :param meth: a function to be decorated and added as a 'meth_read' and
36 'meth_write' to the object.
35 :return: An instantiated DecoratorSample object.37 :return: An instantiated DecoratorSample object.
36 """38 """
3739
@@ -92,6 +94,10 @@
92 self.actions.append('fail_during_write')94 self.actions.append('fail_during_write')
93 raise TypeError('during write')95 raise TypeError('during write')
9496
97 if meth is not None:
98 meth_read = needs_read_lock(meth)
99 meth_write = needs_write_lock(meth)
100
95 return DecoratorSample()101 return DecoratorSample()
96102
97103
@@ -149,6 +155,20 @@
149 self.assertEqual(['lock_write', ('bank', 'bar', 'bing'),155 self.assertEqual(['lock_write', ('bank', 'bar', 'bing'),
150 'unlock_fail'], sam.actions)156 'unlock_fail'], sam.actions)
151157
158 def test_read_lock_preserves_default_str_kwarg_identity(self):
159 a_constant = 'A str used as a constant'
160 def meth(self, param=a_constant):
161 return param
162 sam = create_decorator_sample(self._decorator_style, meth=meth)
163 self.assertIs(a_constant, sam.meth_read())
164
165 def test_write_lock_preserves_default_str_kwarg_identity(self):
166 a_constant = 'A str used as a constant'
167 def meth(self, param=a_constant):
168 return param
169 sam = create_decorator_sample(self._decorator_style, meth=meth)
170 self.assertIs(a_constant, sam.meth_write())
171
152172
153class TestFastDecoratorActions(TestDecoratorActions):173class TestFastDecoratorActions(TestDecoratorActions):
154174
155175
=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt 2011-02-11 17:12:35 +0000
+++ doc/en/release-notes/bzr-2.4.txt 2011-02-14 12:09:20 +0000
@@ -90,6 +90,10 @@
90 ``bzr plugins`` and in crash reports.90 ``bzr plugins`` and in crash reports.
91 (#704195, Martin Pool)91 (#704195, Martin Pool)
9292
93* The "pretty" version of ``needs_read_lock`` and ``needs_write_lock`` now
94 preserves the identity of default parameter values.
95 (Andrew Bennetts, #718569)
96
93* ``bzr dump-btree --raw`` no longer tracebacks on a B-Tree file97* ``bzr dump-btree --raw`` no longer tracebacks on a B-Tree file
94 containing no rows. (Eric Siegerman, #715508)98 containing no rows. (Eric Siegerman, #715508)
9599