Bazaar

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

603461-ensure-no-var-named-message-2.2
Merge into 2.2

Proposed by Parth Malwankar on 2010-07-14

Status:

Merged

Approved by:

Andrew Bennetts on 2010-07-14

Approved revision:

no longer in the source branch.

Merged at revision:

5058

Proposed branch:

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

Merge into:

lp://staging/bzr/2.2

Diff against target:

119 lines (+42/-9)

3 files modified

NEWS (+9/-0)
bzrlib/errors.py (+5/-8)
bzrlib/tests/test_errors.py (+28/-1)

To merge this branch:

bzr merge lp://staging/~parthm/bzr/603461-ensure-no-var-named-message-2.2

High

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Andrew Bennetts		Approve on 2010-07-14
Vincent Ladeuil	2010-07-14	Approve on 2010-07-14
Review via email: mp+29837@code.staging.launchpad.net

Commit message

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

Description of the change

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.

Revision history for this message

Vincent Ladeuil (vila) wrote on 2010-07-14:

This sounds fine, thanks !

review: Approve

Revision history for this message

Andrew Bennetts (spiv) wrote on 2010-07-14:

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

Revision history for this message

Parth Malwankar (parthm) wrote on 2010-07-14:

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

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

Revision history for this message

Andrew Bennetts (spiv) wrote on 2010-07-14:

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.

Revision history for this message

Parth Malwankar (parthm) wrote on 2010-07-14:

> 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.

Revision history for this message

Andrew Bennetts (spiv) on 2010-07-14:

review: Approve

Revision history for this message

Parth Malwankar (parthm) wrote on 2010-07-14:

sent to pqm by email