Merge lp:~parthm/bzr/603461-ensure-no-var-named-message-2.2 into lp:bzr/2.2

[Parth Malwankar]

test to ensure that errors.BzrError subclasses don't use "message" as arg name for __init__ and _fmt (#603461)

This fix adds a test case to prevent bugs like bug #603461 i.e. use of "message" as format variable name in BzrError subclasses is a problem on some Python versions.

The test case checks subclasses of errors.BzrError for two things:
1. __init__ should not have argument named "message"
2. _fmt should not use "message" as a format variable name.

The patch also fixes existing _fmt strings that had "message" variable names in them.

One potentially breaking change could be the use of "message" as __init__ argument in LockError. I have changed this to "msg" and tests for bzr are passing. bzrlib does not use LockError with explicit argument name. A grep of some plugins (bzr-svn, bzr-grep, bzrtools) shows that LockError is not used directly. So I think it should be a safe change. If someone more familiar with use of LockError feels thats not the case I can special case it in the test case.

[Vincent Ladeuil (vila)]

This sounds fine, thanks !

review: Approve

[Andrew Bennetts (spiv)]

 review needs-fixing

This is pretty neat!

The test should be using BzrError.__subclasses__() though, rather than digging
through bzrlib.errors.__dict__. This will be simpler, and find subclasses in
modules other than bzrlib.errors. (This can happen when plugins define errors,
and also the coding guide says that new errors can be defined in modules other
than bzrlib.errors)

-Andrew.

review: Needs Fixing

[Parth Malwankar (parthm)]

> The test should be using BzrError.__subclasses__() though, rather than digging

__subclasses__ is much better. I have updated the patch. Thanks for the review.

[Andrew Bennetts (spiv)]

Oh, I forgot to mention (I was in hurry, sorry), __subclasses__ is new in Python 2.5 :(

So this test should skip if BzrError.__subclasses__ does not exist.

The only other change I'd suggest is this perhaps should be mentioned in API Changes in NEWS.

Apart from those two things this looks good to land for me.

[Parth Malwankar (parthm)]

> Oh, I forgot to mention (I was in hurry, sorry), __subclasses__ is new in
> Python 2.5 :(
>
> So this test should skip if BzrError.__subclasses__ does not exist.
>
> The only other change I'd suggest is this perhaps should be mentioned in API
> Changes in NEWS.
>

The test now skips if __subclasses__ is not present.
Also, added the NEWS entry regarding compatibility. Thanks.

> Apart from those two things this looks good to land for me.

[Andrew Bennetts (spiv)]

review: Approve

[Parth Malwankar (parthm)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-07-13 16:17:52 +0000
+++ NEWS	2010-07-14 09:56:42 +0000
@@ -14,6 +14,12 @@
 Compatibility Breaks
 ********************
 
+* BzrError subclasses no longer support the name "message" to be used
+  as an argument for __init__ or in _fmt format specification as this
+  breaks in some Python versions. errors.LockError.__init__ argument
+  is now named "msg" instead of earlier "message".
+  (Parth Malwankar, #603461)
+
 New Features
 ************
 
@@ -42,6 +48,9 @@
 Testing
 *******
 
+* Unit test added to ensure that "message" is not uses as a format variable
+  name in BzrError subclasses as this conflicts with some Python versions.
+  (Parth Malwankar, #603461)
 
 bzr 2.2b4
 #########

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-07-09 16:16:11 +0000
+++ bzrlib/errors.py	2010-07-14 09:56:42 +0000
@@ -947,11 +947,8 @@
     # original exception is available as e.original_error
     #
     # New code should prefer to raise specific subclasses
-    def __init__(self, message):
-        # Python 2.5 uses a slot for StandardError.message,
-        # so use a different variable name.  We now work around this in
-        # BzrError.__str__, but this member name is kept for compatability.
-        self.msg = message
+    def __init__(self, msg):
+        self.msg = msg
 
 
 class LockActive(LockError):
@@ -1378,12 +1375,12 @@
 
 class WeaveParentMismatch(WeaveError):
 
-    _fmt = "Parents are mismatched between two revisions. %(message)s"
+    _fmt = "Parents are mismatched between two revisions. %(msg)s"
 
 
 class WeaveInvalidChecksum(WeaveError):
 
-    _fmt = "Text did not match it's checksum: %(message)s"
+    _fmt = "Text did not match it's checksum: %(msg)s"
 
 
 class WeaveTextDiffers(WeaveError):
@@ -1437,7 +1434,7 @@
 
 class VersionedFileInvalidChecksum(VersionedFileError):
 
-    _fmt = "Text did not match its checksum: %(message)s"
+    _fmt = "Text did not match its checksum: %(msg)s"
 
 
 class KnitError(InternalBzrError):

=== modified file 'bzrlib/tests/test_errors.py'
--- bzrlib/tests/test_errors.py	2010-07-09 16:16:11 +0000
+++ bzrlib/tests/test_errors.py	2010-07-14 09:56:42 +0000
@@ -16,6 +16,8 @@
 
 """Tests for the formatting and construction of errors."""
 
+import inspect
+import re
 import socket
 import sys
 
@@ -26,11 +28,36 @@
     symbol_versioning,
     urlutils,
     )
-from bzrlib.tests import TestCase, TestCaseWithTransport
+from bzrlib.tests import TestCase, TestCaseWithTransport, TestSkipped
 
 
 class TestErrors(TestCaseWithTransport):
 
+    def test_no_arg_named_message(self):
+        """Ensure the __init__ and _fmt in errors do not have "message" arg.
+
+        This test fails if __init__ or _fmt in errors has an argument
+        named "message" as this can cause errors in some Python versions.
+        Python 2.5 uses a slot for StandardError.message.
+        See bug #603461
+        """
+        fmt_pattern = re.compile("%\(message\)[sir]")
+        subclasses_present = getattr(errors.BzrError, '__subclasses__', None)
+        if not subclasses_present:
+            raise TestSkipped('__subclasses__ attribute required for classes. '
+                'Requires Python 2.5 or later.')
+        for c in errors.BzrError.__subclasses__():
+            init = getattr(c, '__init__', None)
+            fmt = getattr(c, '_fmt', None)
+            if init:
+                args = inspect.getargspec(init)[0]
+                self.assertFalse('message' in args,
+                    ('Argument name "message" not allowed for '
+                    '"errors.%s.__init__"' % c.__name__))
+            if fmt and fmt_pattern.search(fmt):
+                self.assertFalse(True, ('"message" not allowed in '
+                    '"errors.%s._fmt"' % c.__name__))
+
     def test_bad_filename_encoding(self):
         error = errors.BadFilenameEncoding('bad/filen\xe5me', 'UTF-8')
         self.assertEqualDiff(