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 option to include docstrings with stubgen #13284

Merged
merged 33 commits into from Aug 13, 2023

Conversation

chylek
Copy link
Contributor

@chylek chylek commented Jul 29, 2022

Description

Closes #11965.

Add a --include-docstrings flag to stubgen. This was suggested in #11965 along with a use case.
When using this flag, the .pyi files will include docstrings for Python classes and functions and for C extension functions.
The flag is optional and does not change the default stubgen behaviour. When using the flag, the resulting function stubs that contain docstring will no longer be one-liners, but functions without a docstring still retain the default one-liner style.

Example input:

class A:
    """class docstring"""
    def func():
        """func docstring"""
        ...
    def nodoc():
        ...

output:

class A:
    """class docstring"""
    def func() -> None:
        """func docstring"""
        ...
    def nodoc() -> None: ...

Test Plan

Tests testIncludeDocstrings and testIgnoreDocstrings were added to test-data/unit/stubgen.test to ensure the code works as intended. All other tests passed as well.

C extension docstrings are tested using an updated bash script misc/test_stubgenc.sh with test data in test-data/pybind11_mypy_demo/stubgen-include-docs in same fashion as in an already existing test.

Add a --include-docstrings flag to stubgen
This was suggested in python#11965.
When using this flag, the .pyi files will include
docstrings for Python classes and functions and
for C extension functions.
@github-actions

This comment has been minimized.

@chylek chylek marked this pull request as draft July 29, 2022 10:07
@chylek chylek marked this pull request as ready for review July 29, 2022 10:52
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you!

mypy/stubgen.py Outdated Show resolved Hide resolved
test-data/unit/stubgen.test Show resolved Hide resolved
test-data/unit/stubgen.test Show resolved Hide resolved
@chylek
Copy link
Contributor Author

chylek commented Aug 1, 2022

I gave it some more thought and I will have to fix another mistake - fastparse now keeps the docstrings in memory even when nothing uses them. I think I should also add include_docstrings internal flag to mypy.options. The docstrings would be retained only when stubgen (or anything that wants it) sets the flag.

Thanks @sobolevn for you comments, I'll fix it.
I was wondering about the C extension tests as well. There is a shell script that tests stubgenc as part of the github worklow. Perhaps I should create its duplicate that would test the docstrings.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@cnx-tcsikos
Copy link

@hauntsaninja Can you review this PR? I would definitely welcome this feature :)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ws1088
Copy link

ws1088 commented Oct 23, 2022

can you also consider adding variable docstring?

a_variable: int = 0
"""this is the docstring for a_variable"""
class Fields_Obj:
    DefaultValue=None
    """Get/set the default value of the data field"""

https://stackoverflow.com/questions/6060813/how-to-document-fields-and-properties-in-python

@chylek
Copy link
Contributor Author

chylek commented Oct 25, 2022

@ws1088 Interesting - attribute docstrings as PEP 224 were rejected but PEP 257 kind of brings them back. I think the AST doesn't make attribute docstrings accessible, so that wouldn't be an easy change. I think I won't be able to include attribute docstrings.

@FernandoLRubio
Copy link

Is there any ETA for this feature to be merged?

@ilevkivskyi
Copy link
Member

@chylek Could you please resolve merge conflicts? This adds a frequently requested feature, so I would merge this (unless there are objections from other maintainers).

@github-actions

This comment has been minimized.

mypy/stubgenc.py Outdated
function=name,
args=", ".join(args),
ret=strip_or_import(signature.ret_type, module, known_modules, imports),
)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this part completely repeats the other branch? If yes, then please use

if foo:
   bar
baz

instead of

if foo:
    bar
    baz
else:
    baz

Copy link
Member

Choose a reason for hiding this comment

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

To be more precise, in this case you need:

bar
if foo:
    baz

since you always need a type, and docstring should come after (only if the flag is enabled)

@ilevkivskyi
Copy link
Member

@chylek While a docs issue unrelated to your PR is being fixed, could you please fix a style issue I found?

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@ilevkivskyi ilevkivskyi merged commit edbfdaa into python:master Aug 13, 2023
20 checks passed
@chylek
Copy link
Contributor Author

chylek commented Aug 14, 2023

Thank you for merging my pull request and for the guidance and feedback.

yosh-matsuda added a commit to yosh-matsuda/mypy-nanobind that referenced this pull request Oct 13, 2023
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.

stubgen should also include docstrings
7 participants