Skip to content

List all pybind11 exceptions and refer to code. #2671

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

Merged
merged 2 commits into from
Nov 19, 2020
Merged

Conversation

jblespiau
Copy link
Contributor

I wanted to know excacly what existed, because I wanted to raise a
RuntimeError. In practice, it took me quite some time to find where
(a) the pybind11 exceptions are defined, (b) how the C++ exceptions
are converted to Python ones.

At the end, I discovered everything else is converted to RuntimeError,
so I do not need to do anything.

The link is a result to github search, which may be more robust to
moving code around.

I wanted to know excacly what existed, because I wanted to raise a
`RuntimeError`. In practice, it took me quite some time to find where
(a) the pybind11 exceptions are defined, (b) how the C++ exceptions
are converted to Python ones.

At the end, I discovered everything else is converted to RuntimeError,
so I do not need to do anything.

The link is a result to github search, which may be more robust to
moving code around.
Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the end, I discovered everything else is converted to RuntimeError,
so I do not need to do anything.

You probably want to register a nice exception translator anyway, to not have "unknown error" shouted at the user?

Comment on lines 19 to 20
.. [translate_exception] https://github.com/pybind/pybind11/search?q=translate_exception
.. [PYBIND11_RUNTIME_EXCEPTION] https://github.com/pybind/pybind11/blob/master/include/pybind11/detail/common.h
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have other places in our docs where we link to the code. So I'm very wary of adding this. No regular user should immediately go see the code; power-users will be familiar with the code anyway. I would suggest linking to the API reference.

Even more so about the search: everyone can do a search on GitHub, so I see no reason for that link (but do make it a Sphinx reference to the API, if you want).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice, it took me quite some time to find where
(a) the pybind11 exceptions are defined, (b) how the C++ exceptions
are converted to Python ones.

Also, I think neither of these two are user concerns (but only for developers), so I think they should not be in the documentation for the users.

I know this may seem controversial, but I really believe it's useful here for the following reasons:

  • In practice, I have been working a lot with pybind11, so I am way more than a "random" user, but more of an advanced user. And still,even thought I look at the code often, I am not familiar with the code.
  • In practice, the documentation was stale, and I did not got the information I wanted (which was "if I raise an std::runtime_error" what happens?), so I had to try it out and look at the code, and it took me easily 20 minutes. Documentation become obsolete, it's the way it is, and it's way more user-friendly to give some hint where to look, instead of having documentation not in sync in the code, and no way to check. This is particularly true for like enums, or this which was a sequence of if/else if.

I was not able to do a search easily, because I did not know what to search for (i.e. translate_exception and PYBIND11_RUNTIME_EXCEPTION). If there had been one sentence like the one I am adding, it would have taken me very little time.

I am very flexible to change this a way you prefer, but I really would like to have "some way" to hint the user, so next time it's stale, or some user wants to understand the behavior/check the doc is up to date, they can.
I can remove the link, and just say, below the paragraph or below the table "For definitions in the code, see translate_exception and PYBIND11_RUNTIME_EXCEPTION", and let the user do a Github search (but I am wondering why we want to give more work for a curious reader than less.

I can also say "See PYBIND11_RUNTIME_EXCEPTION in common.h" without any link if you prefer (but I don't know why not linking it^^).

Concerning your last suggestion "do make it a Sphinx reference to the API, if you want", maybe it solves the issue, but I do not understand exactly what you mean. For me, these are internal, no need to document them on the API reference, so adding them + linking to them would be even more work, and harder to maintain.

Having read this comment, what change would you be fine to have? (I am very open to any way to make this less visible if you want, like a footnote, moving it, saying "For advanced users, see [...]", but I think removing it will lead to more people spend time like I did.

At the end, I discovered everything else is converted to RuntimeError,
so I do not need to do anything.

You probably want to register a nice exception translator anyway, to not have "unknown error" shouted at the user?

For my use-case, I will be fine with this (it's not something people should catch). I was just explaining the use-case I had that lead me to update the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also note that we discuss over 2 pairs of 2 words in parenthesis. If it's specifically the links, I can remove them, but it's just easier for the reader to click on it. These 4 words can save time to advanced readers, I do not think they will reduce the experience of a beginner reader. Maybe putting these into a footnote would help reduce their visibility?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I am finding such footnote useful, but I applied the suggestion of removing any reference to the code, as It would not be merged with them).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry! Didn't mean to not reply, and definitely not to shut down the conversation over this!

I can definitely see where you're coming from, by the way. I have a similar experience, that at some point, I needed to look into the code to find details! But then that's why docs should be improved, I suppose.

But as you said, that only happens at a more advanced stage, so I would expect these advanced users to start feeling more accustomed to the pybind11 code, anyway.

In practice, the documentation was stale, and I did not got the information I wanted (which was "if I raise an std::runtime_error" what happens?), so I had to try it out and look at the code, and it took me easily 20 minutes. Documentation become obsolete, it's the way it is, and it's way more user-friendly to give some hint where to look, instead of having documentation not in sync in the code, and no way to check.

Yep, that's an issue, definitely. But the only way to fix the documentation is by looking at the code? Any link to the code that's precise enough would probably soon become obsolete as well? And otherwise, the kind of links you had now, will probably stay accurate enough for some time, but are also not very precise, so a "find all"-search in any IDE would provide as much information?
(By the way, these are the things for which we're very happy to point you in the right direction on Gitter. But I can see that sometimes you don't have time to wait for that.)

I was not able to do a search easily, because I did not know what to search for (i.e. translate_exception and PYBIND11_RUNTIME_EXCEPTION). If there had been one sentence like the one I am adding, it would have taken me very little time.

We also really don't want to document these things, I believe. We already had users complain internal convenience macros like NAMESPACE_BEGIN were renamed. Especially in this header-only type of project, it's slightly dangerous to encourage users to dive into the internals :-/
If PYBIND11_RUNTIME_EXCEPTION is meant to be used by users, it should be documented in the API reference and/or elsewhere. I'm not sure, actually, but since it's not documented, it should be considered an implementation detail.

So in conclusion, those are the main things that worry me: mixing user and (non-existing) developer documentation (and this way documenting internals that are currently allowed to change between releases), and adding more opportunities for inconsistencies/outdated documentation (if hidden in links in different sections).

But I do completely agree that the developer documentation is minimal (or non-existent), and that a "quick tour of what each file +- contains" would be great (as long as it's clearly marked on a separate page, under "Development" or so, if you ask me).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, let's see what others think, as far as I'm concerned :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, at least since we don't already link to code, and keeping links to code up-to-date would be practically impossible. For things that are "private", that is, only developers should care, that should be documented in the source but probably not outside of that.

@YannickJadoul
Copy link
Collaborator

In practice, it took me quite some time to find where
(a) the pybind11 exceptions are defined, (b) how the C++ exceptions
are converted to Python ones.

Also, I think neither of these two are user concerns (but only for developers), so I think they should not be in the documentation for the users.

@jblespiau jblespiau force-pushed the doc branch 2 times, most recently from 5d07ab0 to 1e62aa5 Compare November 18, 2020 02:04
For curious readers, see `translate_exception` and
`PYBIND11_RUNTIME_EXCEPTION.
Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to other reviewers (@henryiii, for example) whether they want to bring back some of the extra changes from 1cd9965.

@YannickJadoul YannickJadoul added this to the v2.6.2 milestone Nov 18, 2020
@henryiii henryiii merged commit af8849f into pybind:master Nov 19, 2020
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 19, 2020
@henryiii henryiii added docs Docs or GitHub info and removed needs changelog Possibly needs a changelog entry labels Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Docs or GitHub info
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants