PEP 3134 exception reporting

[Benjamin, 2008-07-13 21:15:39]

Mostly I complained about style things, since I'm not a exception or PEP 3134
expert. Otherwise, the patch seems good.

http://codereview.appspot.com/2448/diff/13/31
File Lib/test/test_raise.py (right):

http://codereview.appspot.com/2448/diff/13/31#newcode281
Line 281: def test_trivial_cycle(self):
Please rename this to test_cycle_broken.

http://codereview.appspot.com/2448/diff/13/31#newcode291
Line 291: def test_nontrivial_cycle(self):
Please rename this to test_reraise_cycle_broken.

http://codereview.appspot.com/2448/diff/13/30
File Lib/test/test_traceback.py (right):

http://codereview.appspot.com/2448/diff/13/30#newcode118
Line 118: class BaseExceptionReportingTests:
Can't this just inherit unittest.TestCase?

http://codereview.appspot.com/2448/diff/13/27
File Python/_warnings.c (right):

http://codereview.appspot.com/2448/diff/13/27#newcode258
Line 258: /* Print "  source_line\n" */
Does the removal of the next line make this comment invalid?

[Antoine Pitrou, 2008-07-13 21:32:03]

http://codereview.appspot.com/2448/diff/13/30
File Lib/test/test_traceback.py (right):

http://codereview.appspot.com/2448/diff/13/30#newcode118
Line 118: class BaseExceptionReportingTests:
On 2008/07/13 21:15:39, Benjamin wrote:
> Can't this just inherit unittest.TestCase?

No, because the test_ methods would be executed while they should only be called
in the derived classes (those which currently also inherit unittest.TestCase).

http://codereview.appspot.com/2448/diff/13/27
File Python/_warnings.c (right):

http://codereview.appspot.com/2448/diff/13/27#newcode258
Line 258: /* Print "  source_line\n" */
On 2008/07/13 21:15:39, Benjamin wrote:
> Does the removal of the next line make this comment invalid?

No, because the indent is added as part of Py_DisplaySourceLine below (with the
last parameter).

Actually this change is just a port of Amaury's 2.6 patch:
http://hg.pitrou.net/public/cpython/trunk/rev/bfd59d49978b

[Benjamin, 2008-07-13 21:35:28]

http://codereview.appspot.com/2448/diff/13/30
File Lib/test/test_traceback.py (right):

http://codereview.appspot.com/2448/diff/13/30#newcode118
Line 118: class BaseExceptionReportingTests:
On 2008/07/13 21:32:03, Antoine Pitrou wrote:
> On 2008/07/13 21:15:39, Benjamin wrote:
> > Can't this just inherit unittest.TestCase?
> 
> No, because the test_ methods would be executed while they should only be
called
> in the derived classes (those which currently also inherit unittest.TestCase).

I think it's better to name the tests explicit in the run_unittest call then.
> 
>

[GvR, 2008-07-14 20:13:23]

I think it's time to check this in, before beta 2 gets released!

http://codereview.appspot.com/2448/diff/37/46
File Python/pythonrun.c (right):

http://codereview.appspot.com/2448/diff/37/46#newcode1374
Line 1374: /* XXX can it still happen in 3.x? */
It shouldn't. Consider replacing with an assert.

http://codereview.appspot.com/2448/diff/37/46#newcode1426
Line 1426: print_exception_recursive(f, cause, seen);
This line and the next are >80 chars.  Also 1435-6 below.

[Antoine Pitrou, 2008-07-14 21:17:01]

(sorry, I forgot to publish that comment)

http://codereview.appspot.com/2448/diff/13/30
File Lib/test/test_traceback.py (right):

http://codereview.appspot.com/2448/diff/13/30#newcode118
Line 118: class BaseExceptionReportingTests:
On 2008/07/13 21:35:28, Benjamin wrote:
> 
> I think it's better to name the tests explicit in the run_unittest call then.

I'm not sure I agree with that. It's too easy to forget adding tests to the
run_unittest call, that's why I find the run_unittest(__name__) syntax much more
pleasant.

This idiom is already used elsewhere, see e.g. test_memoryio.py (the base class
is called Mixin there).

(but perhaps we should discuss that controversy in the unittest thread on
python-dev ;-P)

[Benjamin, 2008-07-14 21:19:40]

On 2008/07/14 21:17:01, Antoine Pitrou wrote:
> 
> I'm not sure I agree with that. It's too easy to forget adding tests to the
> run_unittest call, that's why I find the run_unittest(__name__) syntax much
more
> pleasant.
> 
> This idiom is already used elsewhere, see e.g. test_memoryio.py (the base
class
> is called Mixin there).
> 
> (but perhaps we should discuss that controversy in the unittest thread on
> python-dev ;-P)

Fine, you win. :)

[GvR, 2008-07-15 17:14:45]

You can close this, it's been submitted. :-)