-
Notifications
You must be signed in to change notification settings - Fork 195
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
support exec, eval #299
support exec, eval #299
Conversation
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.
This looks like a great start! Here is some feedback on this first version.
The CMake thing happens sometimes, unfortunately. Not sure what that is all about -- the next time you update the PR, it will likely succeed. |
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.
This looks like it is close to mergeable. One remaining thing that is missing is basic Sphinx documentation that could likely be repurposed from pybind11.
One last thing I remembered: besides copying the docs from nanobind, can you add the function signatures and document then added enum? This should go to |
|
Let's forget about that; I'm getting entangled in segfaults here. To be added as a later step. :) |
Ok. Ready for another review or potentially merge? |
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.
Just a few minor remaining things I noticed.
Always ready! :) (Thanks for the good reviews.) |
Merged, thank you for your work! |
Thanks again for walking me through the process! |
You'll have to be a lot more patient ;). There is a huge issue that I want to fix in an upcoming release (#307), and which requires some stabilization until then. If you cannot wait, just reference a particular commit of this repository (supported by pip, requirements.txt, etc.) |
I took pybind11's
eval.h
plus tests and adapted it for nanobind. I had to removeeval_file
as I couldn't find a stable-ABI equivalent for opening files (done with_Py_fopen_obj
in pybind11).@wjakob I believe this is ready for the review. The test failures seem unrelated.