Fix exception causes in lookup.py #319
Conversation
|
hey there - absolutely, "raise from" is great, but for a PR right now Mako is still supporting Python 2. All is not lost however as SQLAlchemy has a compat layer that does both: py3k version: https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/util/compat.py#L140 py2k version: https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/util/compat.py#L279 (uses py2k-only syntax) |
|
in fact Mako already has this function, so if you can modify your PR to use this function it can go in: https://github.com/sqlalchemy/mako/blob/master/mako/compat.py#L119 |
cool-RR
force-pushed
the
patch-1
branch
2 times, most recently
from
ecddd75
to
d06526a
Compare
|
Cool, done, waiting for your feedback. I do think that compatibility function is a bit weird. For one, I don't see why it would only set |
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision d06526a of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change d06526a: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2041 |
|
I can't figure out how to use your Gerrit. It failed me on this: I fixed it and pushed an amended version, but it's still showing a failure. |
| @@ -10,6 +10,7 @@ | |||
| import stat | |||
|
|
|||
| from mako import exceptions | |||
| from mako import compat | |||
There was a problem hiding this comment.
we actually don't need this import as "reraise" should be available as util.reraise().
There was a problem hiding this comment.
It's not available as util.reraise:
import mako.util
mako.util.reraise
Traceback (most recent call last):
Python Shell, prompt 3, line 1
builtins.AttributeError: module 'mako.util' has no attribute 'reraise'
|
This PR is turning out to be less fun than I thought, because of the Python 2 compatibility layer and the Gerrit integration, so I'm gonna back out. Sorry. |
|
If anyone wants to pick it up instead of me, that'd be cool. |
|
sorry to hear that. the gerrit integration was nothing you need to be concerned with. |
|
mike bayer (zzzeek) wrote: View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2041 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2041 has been abandoned. That means that at least for the moment I need to close this pull request. Sorry it didn't work out :( |
|
Michael Bourke (bourke) wrote: View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2041 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2041 has been restored. That means I can reopen this pull request! Hooray :) |
|
Michael Bourke (bourke) wrote: Now that main is Python 3 only, this was straightforward to implement. Needs a review though. View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3337 |
|
Ram Rachum (cool-RR) wrote: I'm happy to see that you picked it up Michael. Thank you. View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3337 |
|
mike bayer (zzzeek) wrote: we are on main now View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2041 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2041 has been abandoned. That means that at least for the moment I need to close this pull request. Sorry it didn't work out :( |
|
mike bayer (zzzeek) wrote: i made some changes View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3337 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3337 has been merged. Congratulations! :) |
Mako now performs exception chaining using ``raise from``, correctly identifying underlying exception conditions when it raises its own exceptions. Pull request courtesy Ram Rachum. Additionally includes cleanup of the test suite to include better exception fixtures. Closes: #319 Pull-request: #319 Pull-request-sha: d06526a Additionally: Fixes: #348 Change-Id: Ibb2864de822bf4b63adf22a6bb32cf0758d296bd
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_errorneeds 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 ofraise fromtells Python to put a more accurate message between the tracebacks. Instead of this:You'll get this:
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!