-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Exceptions raised in nim catchable as native python Exception subclasses #300
base: master
Are you sure you want to change the base?
Make Exceptions raised in nim catchable as native python Exception subclasses #300
Conversation
PyExc_IOError*: PPyObject # in Python 3 IOError *is* OSError | ||
PyExc_ValueError*: PPyObject | ||
PyExc_AttributeError*: PPyObject | ||
# PyExc_BlockingIOError # no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mapped many new exceptions in from https://docs.python.org/3/c-api/exceptions.html#standard-exceptions, but most of these probably will not be required as they don't directly map to a standard library nim exception as per https://nim-lang.org/docs/exceptions.html.
Before I move this out of draft, I'll make a point of removing the commented/unmapped entries.
I didn't delete any, but I made a point of alphabetizing them
@@ -212,6 +253,7 @@ proc loadPyLibFromModule(m: LibHandle): PyLib = | |||
|
|||
load Py_BuildValue, "_Py_BuildValue_SizeT" | |||
load PyTuple_New | |||
load PyTuple_Pack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.python.org/3.13/c-api/tuple.html
It's also part of the stable c API, and has been present since at least 3.8 (probably much longer).
Added it as a convenience function for myself.
tests/tnimfrompy.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notably: the pattern of exception raising/catching in this file doesn't technically cover the case where the nim code failed to raise an exception at all... I might need to add some boilerplate to account for that.
Though depending on what versions of python we aim to support, would it be sensible to re-write these using an actual unit testing framework? Probably makes more sense in a different PR but 🤷🏾♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, just add doassert(false) at the end of each try block. No need for frameworks
nim
catchable as native Python Exceptions
tests/nimfrompy.nim
Outdated
proc attributeError(): string {.exportpy} = | ||
# FieldDefect | ||
var obj: MyObj | ||
obj.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly, I'm not 100% convinced this makes sense to map FieldDefect
to an AttributeError
, but I'm curious what others think
Fixes #207
Old behavior
Whenever an Exception was raised nim code, the
nimpy
python module would raise a correspondingnimpy.<exceptionName>
. This Exception object is a subclass ofBaseException
in python.Why is it a problem?
except BaseException
is generally a code smell in python since it catchesKeyboardInterrupt
raised by ctrl-cexcept Exception
, which is also a code smell but at least far safer, doesn't even work.KeyError
raised by nim is still animpy.KeyError
in python)New behavior
<nimpymodule>.NimPyException
except BaseException
and checks thee.__class__.__name__
or similar should continue to work as it did before.TODO before moving out of draft
TypeErrors
) fit into the general__mro__
of every other one to ensure cleanlinessAddendum
While this PR is a WIP, I'm temporarily disallowing
Allow edits by maintainers
so I don't run into anygit disasters
, but I will enable it immediately upon request by a maintainer, or as soon as this is out of draft!Please feel free to give a preliminary review, including critical feedback, as this is my very first contribution to a nim open source project! My domain expertise is in python, but I've been having a lot of fun working with nimpy on various projects, so making this sort of fix was important to me, but I'm by no means claiming that my nim code is at all perfect, so feedback is very welcome!