Skip to content
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

Add getters for exception type, value and traceback #1641

Merged
merged 1 commit into from
May 3, 2019

Conversation

martinRenou
Copy link
Contributor

In order to retrieve the exception type, value and trace, we can use sys.exc_info() in Python. But using pybind11 it's not that easy currently.
When an exception occurs in Python code it's directly converted to py::error_already_set and it holds the error. Which means that if we want to retrieve the type, value and trace we first need to restore the error using the .restore() method, and then call PyErr_Fetch, which is not ideal.
I added accessors to objects describing the error in py::error_already_set.

Please tell me what you think :)

@YannickJadoul
Copy link
Collaborator

@martinRenou Shouldn't/couldn't this be const object &get_type() const { return type; }, as C++-style to avoid refcounting in the copy constructor when it's not necessary to copy the object object?

@YannickJadoul
Copy link
Collaborator

@martinRenou Sorry to bother you again, but before I can rebase and force-push #1651, the methods also need to be const (so I can call it on the caught const py::error_already_set & in a catch block).

@martinRenou
Copy link
Contributor Author

Don't worry :) I'll do it. Do you have an idea why the build is not passing?

@YannickJadoul
Copy link
Collaborator

I'm looking into that now, actually. On Travis, it's got something to do with a dead link, it seems. Nothing in this PR, I'd guess :-)

@YannickJadoul
Copy link
Collaborator

Although ... the current build failure is also partly due to this PR, I think, since I'm getting error messages on "duplicate const".
Probably this const object& get_type() { return type; } const instead of const object& get_type() const { return type; } typo?

@martinRenou
Copy link
Contributor Author

Oh yeah sorry... I did that too quickly with vim without really checking what I was doing

@YannickJadoul
Copy link
Collaborator

Great, thanks! :-)
Rebased #1651 onto this, already.

About the failing tests: #1642 is fixing the broken build and solving the Travis part.

@zhihaoy
Copy link

zhihaoy commented Jan 7, 2019

Unless someone whats to bikeshed it, I think this is good, because I've started to use it (I thought about letting vcpkg to patch it, at least in our team's vcpkg clone).

@martinRenou
Copy link
Contributor Author

Any chance to merge this?

@wjakob
Copy link
Member

wjakob commented Apr 25, 2019

This seems reasonable. However, just a comment regarding the naming: we haven't so far used get_ as prefix for getters in this library.

@wjakob
Copy link
Member

wjakob commented Apr 25, 2019

actually, scratch that :) -- there are quite a few prefixed get_.

@martinRenou
Copy link
Contributor Author

I can change it to type, value and trace if you prefer :)

@wjakob
Copy link
Member

wjakob commented Apr 25, 2019

Hm, I think I would prefer them to be named just value(), trace(), etc. (you will probably have to change the name of the members then).

There is one slight inconsistency with handle::get_type which actually does use a get_ prefix, but elsewhere we generally omit it.

@martinRenou martinRenou force-pushed the add_exception_getters branch 2 times, most recently from 2a17a37 to 846ede2 Compare April 25, 2019 13:27
@martinRenou
Copy link
Contributor Author

@wjakob Hope it's fine now :)

@martinRenou
Copy link
Contributor Author

Any chance to get this in? @wjakob. I guess the travis job not passing is unrelated.

@apollo13
Copy link
Contributor

apollo13 commented May 3, 2019

I am running with this patch locally, works fine so far

@wjakob
Copy link
Member

wjakob commented May 3, 2019

Merged, thanks!

@wjakob wjakob merged commit 35045ee into pybind:master May 3, 2019
@martinRenou martinRenou deleted the add_exception_getters branch May 3, 2019 12:35
@martinRenou
Copy link
Contributor Author

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants