Skip to content

[FEATURE] Allow method_descriptors to be serialized as methods #103

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

Conversation

jmrgibson
Copy link
Contributor

Hi,

I'm trying to add support for python methods created using pyo3. These methods are "built-in" methods, however they can still expose the __doc__ attribute and a function signature via __text_signature__.

The current implementation detects method_descriptors (which is what most native functions end up as), and serializes them as regular methods (since from a caller point of view, they act the same). If the user doesn't provide the __text_signature__ attribute, inspect.signature will error out, so I made it optional, as I still think it would be useful to have the docstring in the resulting documentation if it exists.

Please let me know if this is the right direction for this approach. There are other modifications coming to mkdocstrings too, as it currently doesn't populate functions/methods with __text_signature__ or with no signature very well.

Thanks,

@jmrgibson jmrgibson force-pushed the user/jmrgibson/add_method_descriptor_support branch from 7554f60 to 6fdaa38 Compare April 22, 2021 03:50
@jmrgibson jmrgibson changed the title Allow method_descriptors to be serialized as methods [FEATURE] Allow method_descriptors to be serialized as methods Apr 22, 2021
@jmrgibson jmrgibson force-pushed the user/jmrgibson/add_method_descriptor_support branch from 6fdaa38 to a97f676 Compare April 22, 2021 04:45
Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Looking good 🙂
Since inspect itself provides the ismethoddescriptor method, I'm inclined to merge this.
Could you add a test for this change? Is it feasible?

@jmrgibson
Copy link
Contributor Author

Hmm. I don't really want to add rust + pyo3 as a dependency, but I should be able to make a test that creates a method descriptor using pure python. I'll dig through python docs and have a test sometime this week.

@jmrgibson jmrgibson force-pushed the user/jmrgibson/add_method_descriptor_support branch from a97f676 to 5af495e Compare April 26, 2021 23:24
@jmrgibson jmrgibson force-pushed the user/jmrgibson/add_method_descriptor_support branch from 5af495e to e52eb0b Compare April 26, 2021 23:25
@jmrgibson
Copy link
Contributor Author

I was able to use the built-in int.__add__, which was very convenient. Test added.

@pawamoy pawamoy merged commit 8e1b1b2 into mkdocstrings:master May 1, 2021
@pawamoy
Copy link
Member

pawamoy commented May 1, 2021

Thanks again!

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