Fix exception causes in relations.py

[cool-RR]

I recently went over Matplotlib, Pandas and NumPy, fixing a small mistake in the way that Python 3's exception chaining is used. If you're interested, I can do it here too. I've done it on just one file right now.

The mistake is this: In some parts of the code, an exception is being caught and replaced with a more user-friendly error. In these cases the syntax raise new_error from old_error needs to be used.

Python 3's exception chaining means it shows not only the traceback of the current exception, but that of the original exception (and possibly more.) This is regardless of raise from. The usage of raise from tells Python to put a more accurate message between the tracebacks. Instead of this:

During handling of the above exception, another exception occurred:

You'll get this:

The above exception was the direct cause of the following exception:

The first is inaccurate, because it signifies a bug in the exception-handling code itself, which is a separate situation than wrapping an exception.

Let me know what you think!

[rpkilby]

This looks great. Happy to merge this as-is, or can wait for you to comb through other files.

Also, here's an explanation of the underlying mechanics.

[cool-RR]

I'm happy you like it. I'd rather you merge this one first, and I'll do the rest in another PR.

[carltongibson]

Please don't do this. There was a big discussion on Django-developers about this very topic. It's unnecessary and adds additional noise to the code.

[carltongibson]

This was the discussion: https://groups.google.com/d/msg/django-developers/ibEOt3A9c2M/sOL7YV5LFwAJ

I'm going to close this.

[carltongibson]

Sorry, getting late, I've misread this — it's actually adding chaining, so adding contextual information. Reopening.

[carltongibson]

The question here is whether the longer double traceback is providing useful extra info? Is it just more noise? The exception raised has the message you need to address no?

[cool-RR]

I'll give a detailed answer tomorrow.

[carltongibson]

Thanks @cool-RR. With time to pause and think about it again, I believe my original reaction was correct:

• All exceptions are chained unless this is stopped using raise ... from None to suppress the exception chaining.
• Currently we have implicit chaining, so-called.
• Add the from e makes it explicit.

In both cases the traceback with output the stack for both exceptions. In the implicit case you get "During handling of the above exception, another exception occurred" dividing them in the second "The above exception was the direct cause of the following exception". But the information provided is the same. There's no advantage to the different text.

There's no benefit to automatically (i.e. always, without deciding on a case-by-case basis) adding the from e. It's just more code. It adds nothing.

The use-case for from e is when you want to expose a particular exception from a group of group of chained exceptions, e.g. one that's a few in but is the real issue, and the one you want to surface to user the handle. (The other use-case is the from None, which folks use to make the stack trace clearer to read, by suppressing the uninteresting output.)

It's not the case here that we're doing anything fancy: we just have two exceptions and they'll be (implicitly) chained by default. Sprinkling these from e everywhere isn't a win — they should be used selectively, when there's a call.

Re-reading it, this is just to repeat the discussion on the Django Developers thread. @cool-RR I'm curious why you're keen to add them: what do you think they're adding?

Thanks. Sorry for the back-and-forth: it was late in the afternoon (later now, but that's a different story...) and I didn't want it rushed in: I think it would be a mistake.

[rpkilby]

That's fair. The wording of the explicit form reads as more "correct" to me, but at the end of the day the same context/information is being displayed. I'd agree that the benefit isn't really worth it.

[cool-RR]

I respect your decision to reject this change.

@cool-RR I'm curious why you're keen to add them: what do you think they're adding?

I can answer this question, but there wouldn't be any new information in my answer. I think they're adding a distinction between "the exception below is a more user-friendly representation of the exception above" and "while handling the exception above, there was another unrelated failure, and its exception is below."

When I'm debugging I care about that distinction and I want to know which of these cases I'm dealing with.

But I think we already dug deep enough into that and we have different views.