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

docstrings for compiled builtin modules #420

Merged
merged 12 commits into from
Jun 17, 2024
Merged

docstrings for compiled builtin modules #420

merged 12 commits into from
Jun 17, 2024

Conversation

DetachHead
Copy link
Owner

@DetachHead DetachHead commented Jun 16, 2024

fixes #160

a few things we need to address before merging tho:

  • this currently only generates stubs for the python version & platform build script is running on. need to set up github actions to run them on every platform and python version, then combine them at the end
  • currently this PR drops support for python 3.8 because docify doesn't support it. we should probably wait until 3.8 goes eol before merging this, or do you think it's feasible to add 3.8 support @AThePeanut4?
    • edit: turned out this was as easy as downgrading libcst to 1.1.0
  • doesn't work on the playground because it uses the npm package, and docify is currently only run in the pypi package
  • doesn't work when using the version of basedpyright bundled with the vscode extension for the same reason. (though not an issue if you have the pypi package installed and are using the "fromEnvironment" importStrategy)
  • document it, and also mention the docify repo if users need to generate docs for their own stubs
  • add tests

@DetachHead DetachHead requested a review from KotlinIsland June 16, 2024 07:52
@DetachHead DetachHead force-pushed the builtin-docstrings branch 3 times, most recently from d59b699 to f246b62 Compare June 16, 2024 09:30
@DetachHead DetachHead force-pushed the builtin-docstrings branch 23 times, most recently from 5135d22 to 1939267 Compare June 17, 2024 05:50
@DetachHead DetachHead force-pushed the builtin-docstrings branch 2 times, most recently from 4141e0a to d8257fd Compare June 17, 2024 10:28

This comment has been minimized.

@DetachHead DetachHead force-pushed the builtin-docstrings branch 4 times, most recently from 92ded14 to 53200cf Compare June 17, 2024 11:03

This comment has been minimized.

@DetachHead DetachHead force-pushed the builtin-docstrings branch from 53200cf to 5e4b5a9 Compare June 17, 2024 11:24
Copy link
Contributor

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

@DetachHead DetachHead merged commit 1c2c80f into main Jun 17, 2024
13 checks passed
@DetachHead DetachHead deleted the builtin-docstrings branch June 17, 2024 11:46
@AThePeanut4
Copy link

AThePeanut4 commented Jun 17, 2024

@DetachHead python 3.8 has a few other minor issues besides just the libcst version, I've pushed a fix to support 3.8 fully. I think you missed 3.8 in based_build/generateAllDocstubs.sh.

I've also added a run function that accepts kwargs rather than a list of string arguments, for easier usage when running programmatically.

@DetachHead
Copy link
Owner Author

oh oops, nice catch. thanks!

fixed in #423

@jfcherng
Copy link
Contributor

jfcherng commented Jun 19, 2024

I think docstrings can be added to the whole stdlib. Current it seems to be only done with builtin.pyi?

@DetachHead
Copy link
Owner Author

DetachHead commented Jun 19, 2024

yeah i did that on purpose. i only made it run on compiled builtins because i figured running it on the entire stdlib would unnecessarily make it slower to build and increase the risk of something going wrong. since pyright can already see docstrings from all the other stdlib modules anyway i didn't see the point.

feel free to raise an issue if you disagree. i'm open to changing this behavior in the future if that's what people want

@jfcherng
Copy link
Contributor

Ah, got it. We only need it for compiled modules. NVM.

@jfcherng
Copy link
Contributor

jfcherng commented Jun 19, 2024

Ah, got it. We only need it for compiled modules. NVM.

Hmm... doesn't seem to be this case.

import re

if m := re.search('foo', 'foo'):
    aa = m.group()
    #      ^^^^^ no docstring for `group`
>>> import inspect, re; m = re.search('foo', 'foo'); inspect.getdoc(m.group)
'group([group1, ...]) -> str or tuple.\nReturn subgroup(s) of the match by indices or names.\nFor 0 returns the entire match.'

There can a lot of built-in things still implemented in C. So I would still consider that dosctrings should be generated for the whole stdlib typeshed-fallback.

@DetachHead
Copy link
Owner Author

oh man that's annoying, i didn't know there were modules that are only partially compiled. also it looks like pylance has the same issue

i opened #428 to track this

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.

Docstrings for standard library
3 participants