-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 option for enable/disable enum members in docstring. #2768
Conversation
Two things:
|
I wanted to put this in the same place as the other docstring related options (
There is still the docstring of the enum class itself ( |
Yeah, I'm not 100% convinced of either approach, but I thought it's worth bringing up). If there are no further complaints, I'm happy to keep the global option, though.
I believe that if you don't set |
You are right, I've just tested this and it works! |
Thanks, that's already better, I think :-)
Just to add to future discussion (I'm still not saying this is preferred): if we want to get around this, another option is to come up with some custom tag like |
This will also fix #2275 when disabling the enum members docstring (aka not setting # include <pybind11/pybind11.h>
namespace py = pybind11;
enum Numbers {
zero,
one
}
PYBIND11_MODULE(numbers, m) {
py::options options;
options.disable_enum_members_docstring();
py::enum_<Numbers>e(m, "Numbers");
e
.value("zero", zero)
.value("one", one)
.export_values();
e.attr("__doc__") = "test";
} |
Thanks for checking that as well, @knarfS! |
051afd1
to
18be530
Compare
Looks good to me, based on the understanding that this PR adds an option with a default that leaves the existing behavior unchanged, unless the option is actually used. Is that correct? The only thing missing are tests and possibly a couple lines in the documentation (I'd look for documentation for existing options as a starting point). |
491592b
to
71b9527
Compare
Yes, the current docstring behavior is untouched. I've added a test to secure that. I've also added a section about the undocumented option |
6ff7ce7
to
38d8350
Compare
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.
Nice, thank you!
Thank you @rwgk for reviewing and approving. I updated the PR with your suggestions. |
Great, thanks! I'll wait a day or so to give @Skylion007 I chance to review, too. |
include/pybind11/pybind11.h
Outdated
dict entries = arg.attr("__entries"); | ||
if (((PyTypeObject *) arg.ptr())->tp_doc) { | ||
docstring | ||
+= std::string(((PyTypeObject *) arg.ptr())->tp_doc) + "\n\n"; |
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.
nit: reinterpret_cast ?
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.
Applied in commit c2456d9
include/pybind11/pybind11.h
Outdated
auto comment = kv.second[int_(1)]; | ||
docstring += "\n\n " + key; | ||
if (!comment.is_none()) { | ||
docstring += " : " + (std::string) pybind11::str(comment); |
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.
nit: static_cast or .cast<std::string>
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.
Also applied in Applied in commit c2456d9
I also changed the code to systematically use +=
.
Sorry, missed this in my Github notifications. |
I went ahead with a rebase & applying the review suggestions, to get this into the next smart_holder update. Waiting for the CI. |
The only one CI failure is a known flake (test_gil_scoped DEADLOCK), definitely unrelated to this RP. |
Thanks @rwgk for merging! Not sure, but maybe #2275 can be also closed now (see test above). |
@AWhetter Is there a chance you could confirm & update or close #2275? |
This does indeed fix that issue! |
Thanks @AWhetter! |
Description
This adds the option to disable/enable the enum members listed in the docstring (default is enabled). This is useful, when the documentation tool is able to generate documentation for class variables, so the enum members / class variable aren't listed twice in the documentation. An other benefit of using the features of the documentation generator is, that the members are linkable in the documentation.
I'm using pdoc3 to generate the documentation of an embedded python module, but the above also applies partially to pydoc. No idea about sphinx, I wasn't able to generate the documentation programmatically with sphinx...
I'd be happy to add that option to the documentation, but need a hint to which section this would fit best. There is a chapter about sphinx, but that doesn't exactly match.
Suggested changelog entry: