Re: Ticket #3349 patch review

[Johannes Dollinger]

Am 15.12.2009 um 19:57 schrieb Andrew Durdin:

> On Dec 14, 11:00 pm, ab  wrote:
>>> `wrap_and_raise()` will appear in the traceback, `raise
>>> wrap_exception(SomeException())` would be cleaner.
>>
>> I like that
>
> But you must use the three-argument `raise` statement to supply your
> own traceback in Python 2.x. You could dispense with the function
> entirely if you're happy to repeat `import sys; raise NewException(),
> None, sys.exc_info()[2]` instead--although then you lose some
> information, see below.

I pictured the solution to be closer to PEP 3109 / Java: The wrapped  
exception as an attribute on the new exception, with the traceback  
attached in `exp.__cause__.__traceback__`. That's where my  
`WrappingException(.., wrap=True)` came from.
I don't like the mangled tracebacks, but you're right - the reference  
cycle-free python 2.x solution probably is to use the tree-argument  
raise statement.

>>> Your patch seems to swallow the new exception's traceback now  
>>> instead
>>> of the causing traceback. That might be good in some situations, but
>>> generally both should be preserved.
>
> No; the only part of the traceback lost is the `raise` line within
> `wrap_and_raise`; the remainder of the traceback which would
> correspond to all but the `raise` line of the new exception's
> traceback is preserved. But if you weren't using the `wrap_and_raise`
> function, you would lose the line of the traceback where the new
> exception was raised. Put the following code in a python script and
> compare the tracebacks when it calls wrapped(), unwrapped(), or
> manually_wrapped():

My mistake, of course you're right.

__
Johannes

--

You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Ticket #3349 patch review

[Andrew Durdin]

While I think of it: One thing this patch should address is updating
the `contributing` page  to mention calling wrap_and_raise whenever
you are "catching an exception
arising from user-supplied code and then raising a different
exception".

Andrew.

--

You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Ticket #3349 patch review

[Andrew Durdin]

On Dec 14, 11:00 pm, ab  wrote:
> > `wrap_and_raise()` will appear in the traceback, `raise  
> > wrap_exception(SomeException())` would be cleaner.
>
> I like that

But you must use the three-argument `raise` statement to supply your
own traceback in Python 2.x. You could dispense with the function
entirely if you're happy to repeat `import sys; raise NewException(),
None, sys.exc_info()[2]` instead--although then you lose some
information, see below.


> > Your patch seems to swallow the new exception's traceback now instead
> > of the causing traceback. That might be good in some situations, but
> > generally both should be preserved.

No; the only part of the traceback lost is the `raise` line within
`wrap_and_raise`; the remainder of the traceback which would
correspond to all but the `raise` line of the new exception's
traceback is preserved. But if you weren't using the `wrap_and_raise`
function, you would lose the line of the traceback where the new
exception was raised. Put the following code in a python script and
compare the tracebacks when it calls wrapped(), unwrapped(), or
manually_wrapped():


def wrap_and_raise(new_exception):
  import sys
  exc_class, exc, tb = sys.exc_info()
  if issubclass(exc_class, Exception):
raise new_exception, None, tb
  else:
raise new_exception

def valueerror():
  raise ValueError("This is a ValueError")

def wrapped():
  try:
valueerror()
  except:
wrap_and_raise(StandardError("This error normally hides the
original error"))

def unwrapped():
  try:
valueerror()
  except:
raise StandardError("This error hides the original error")

def manually_wrapped():
  import sys
  try:
valueerror()
  except:
raise StandardError("This error normally hides the original
error"), None, sys.exc_info()[2]

# Try each of these
wrapped()
# unwrapped()
# manually_wrapped()



> > Better yet, make all exceptions that are used to reraise other  
> > exceptions a subclass of WrappingException (pick a better name) that  
> > either takes a `cause=exc` or `wrap=True` kwarg. This way, you don't  
> > have to add imports everywhere.
>
> I don't like that. An invalid template exception might be "wrapping"
> sometimes, but not others.

TemplateSyntaxError is an obvious example. Also, if you do this you're
still not preserving the original traceback.


> Another question: how should the tests for this ticket be written?

I'm not sure (which is why I didn't write any tests originally).
Obviously you'd raise an exception, catch it, and wrap_and_raise
another. I suppose you could examine the output of
`traceback.format_exc()` or one of its other functions to see if the
original exception is now mentioned in the traceback.

Andrew.

--

You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Ticket #3349 patch review

[ab]

> `wrap_and_raise()` will appear in the traceback, `raise  
> wrap_exception(SomeException())` would be cleaner.

I like that

> Better yet, make all exceptions that are used to reraise other  
> exceptions a subclass of WrappingException (pick a better name) that  
> either takes a `cause=exc` or `wrap=True` kwarg. This way, you don't  
> have to add imports everywhere.

I don't like that. An invalid template exception might be "wrapping"
sometimes, but not others.

Another question: how should the tests for this ticket be written?

--

You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Ticket #3349 patch review

[Johannes Dollinger]

Am 14.12.2009 um 10:53 schrieb Andrew Durdin:

> I'm the author of the current patch; I'll just add a bit of
> background.
>
> On Dec 12, 10:18 pm, ab  wrote:
>>
>> 1. Scope -- the patch generalizes the issue and addresses it
>> throughout Django. Are people ok with that?
>
> Since the problem of raising new exceptions and losing the original
> one seemed systemic throughout much of Django, I thought it
> appropriate to make the patch address all known instances of this
> class of problem, and not just the one instance in the original
> complaint.

Yes, please.

>> 2. Design -- The wrap_and_raise function needs to be used not only at
>> the site of the problem, but also possibly at a deeper level in the
>> call stack as well. In this particular case, it needs to be used in
>> both `get_library` and `load` to address the original issue.
>
> Correct. It needs to be used wherever Django is catching an exception
> arising from user-supplied code and then raising a different
> exception.
>
>> 3. Design -- Seeing the ImportError's traceback is indeed helpful in
>> identifying the source of the problem. But, it's also kind of weird  
>> to
>> have the traceback of an ImportError (ending with the line "import
>> notthere" or whatever) associated with a different exception.
>
> I'll just clarify for anyone that hasn't applied the patch and tried
> it: with the patch in place, the traceback will still show down to the
> TemplateSyntaxError, but instead of stopping there will also continue
> down to where the ImportError originated. Yes, it's a little weird;
> Python 3.0 handles this whole situation much better with "raise ...
> from ..." which is intended for use in precisely these situations.

`wrap_and_raise()` will appear in the traceback, `raise  
wrap_exception(SomeException())` would be cleaner.

Better yet, make all exceptions that are used to reraise other  
exceptions a subclass of WrappingException (pick a better name) that  
either takes a `cause=exc` or `wrap=True` kwarg. This way, you don't  
have to add imports everywhere.

Your patch seems to swallow the new exception's traceback now instead  
of the causing traceback. That might be good in some situations, but  
generally both should be preserved.
__
Johannes

--

You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Ticket #3349 patch review

[Andrew Durdin]

I'm the author of the current patch; I'll just add a bit of
background.

On Dec 12, 10:18 pm, ab  wrote:
>
> 1. Scope -- the patch generalizes the issue and addresses it
> throughout Django. Are people ok with that?

Since the problem of raising new exceptions and losing the original
one seemed systemic throughout much of Django, I thought it
appropriate to make the patch address all known instances of this
class of problem, and not just the one instance in the original
complaint.

> 2. Design -- The wrap_and_raise function needs to be used not only at
> the site of the problem, but also possibly at a deeper level in the
> call stack as well. In this particular case, it needs to be used in
> both `get_library` and `load` to address the original issue.

Correct. It needs to be used wherever Django is catching an exception
arising from user-supplied code and then raising a different
exception.

> 3. Design -- Seeing the ImportError's traceback is indeed helpful in
> identifying the source of the problem. But, it's also kind of weird to
> have the traceback of an ImportError (ending with the line "import
> notthere" or whatever) associated with a different exception.

I'll just clarify for anyone that hasn't applied the patch and tried
it: with the patch in place, the traceback will still show down to the
TemplateSyntaxError, but instead of stopping there will also continue
down to where the ImportError originated. Yes, it's a little weird;
Python 3.0 handles this whole situation much better with "raise ...
from ..." which is intended for use in precisely these situations.

Andrew

--

You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Ticket #3349 patch review

[Jeremy Dunck]

On Sat, Dec 12, 2009 at 4:18 PM, ab  wrote:
> I'd like some opinions on the existing patch for 
> http://code.djangoproject.com/ticket/3349.
> This ticket ("If an ImportError occurs within some loaders a rather
> confusing exception is raised") seeks to address the difficulty of
> debugging ImportErrors raised in template tag library.
...
> 3. Design -- Seeing the ImportError's traceback is indeed helpful in
> identifying the source of the problem. But, it's also kind of weird to
> have the traceback of an ImportError (ending with the line "import
> notthere" or whatever) associated with a different exception.

This may be helpful?
http://blog.ianbicking.org/2007/09/12/re-raising-exceptions/

--

You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.