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.

@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

@ilevkivskyi ilevkivskyi Aug 12, 2023

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
19 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
JelleZijlstra pushed a commit that referenced this pull request Nov 29, 2023
This addresses several regressions identified in
#16486

The primary regression from #15770 is
that pybind11 properties with docstrings were erroneously assigned
`typeshed. Incomplete`.

The reason for the regression is that as of the introduction of the
`--include-docstring` feature
(#13284, not my PR, ftr),
`./misc/test-stubgenc.sh` began always reporting success. That has been
fixed.

It was also pointed out that `--include-docstring` does not work for
C-extensions. This was not actually a regression as it turns out this
feature was never implemented for C-extensions (though the tests
suggested it had been), but luckily my efforts to unify the pure-python
and C-extension code-paths made fixing this super easy (barely an
inconvenience)! So that is working now.

I added back the extended list of `typing` objects that generate
implicit imports for the inspection-based stub generator. I originally
removed these because I encountered an issue generating stubs for
`PySide2` (and another internal library) where there was an object with
the same name as one of the `typing` objects and the auto-import created
broken stubs. I felt somewhat justified in this decision as there was a
straightforward solution -- e.g. use `list` or `typing.List` instead of
`List`. That said, I recognize that the problem that I encountered is
more niche than the general desire to add import statements for typing
objects, so I've changed the behavior back for now, with the intention
to eventually add a flag to control this behavior.
@chdominguez
Copy link

@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.

Any news on attribute docstrings?

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
8 participants