Skip to content

Fix docstrings of enum methods #2095

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
Apr 26, 2020

Conversation

YannickJadoul
Copy link
Collaborator

#1511 broke the docstrings of the __repr__, __add__, etc. methods of enum_ types. E.g., the docstring of a __repr__ now looks like this:

>>> example.AnEnum.__repr__.__doc__
'(self: handle) -> str\n'

This PR fixes that by passing the name to the cpp_function constructors, in the same was as class_::def would do:

>>> example.AnEnum.__repr__.__doc__
'__repr__(self: handle) -> str\n'

I also converted __setstate__ and __getstate__ to pickle, to get rid of the warnings about old- vs. new-style constructors.

YannickJadoul added a commit to YannickJadoul/Parselmouth that referenced this pull request Jan 25, 2020
@wjakob
Copy link
Member

wjakob commented Jan 27, 2020

All the changes look good for me, except the changes regarding hash and getstate. I remember at the time intentionally getting rid of pickle() to avoid unneeded code, which you now added back. Can we stick with how it was previously?

@YannickJadoul
Copy link
Collaborator Author

The reason I did this is that by adding the name to the original __setstate__, it would start complaining about "using an old-style placement-new 'setstate' which has been deprecated", and I thought the one function wouldn't matter and even avoid a second call from C++ -> Python -> C++ by calling __int__ is called in __getstate__.

But this latest commit should fix it.

One minor concern is that the docstrings still show int_ instead of int, but if I can easily fix that, I'll open a new PR.

YannickJadoul added a commit to YannickJadoul/Parselmouth that referenced this pull request Jan 27, 2020
@wjakob
Copy link
Member

wjakob commented Apr 26, 2020

LGTM, thanks!

@wjakob wjakob merged commit 503231d into pybind:master Apr 26, 2020
@wjakob
Copy link
Member

wjakob commented Apr 26, 2020

FYI: I removed the use of std::move in __setstate__, that seemed a bit overkill for an enum.

@YannickJadoul
Copy link
Collaborator Author

FYI: I removed the use of std::move in __setstate__, that seemed a bit overkill for an enum.

Oh right, it's just a primitive underlying type, anyway. That was just a remainder of copy-pasting the __setstate__ implemenation, I suppose.

@YannickJadoul YannickJadoul deleted the enum-method-cpp_function-names branch April 26, 2020 16:31
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.

2 participants