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

initial docstring support #515

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bagel897
Copy link
Contributor

@bagel897 bagel897 commented Nov 3, 2022

#504

BREAKING CHANGES:

  1. Schema change on database (fixed by auto-versioning)
  2. SearchResult (only for the full API, not the simple one)

Other concerns:

  • Repeatedly stores module docstrings (we should probably move it to a separate table)
  • Increases the DB size (5mb vs 20mb same env)
  • There's some limitations on what we can and cannot get docstrings for (see typing). Since we use AST for these its incomplete and not always accurate (due to decorators or such)
  • Slightly increases cache time 1.33 sec to 1.43 sec on large env

Preview

image

Side note: I really love the ORM you made, its very cool

Checklist (delete if not relevant):

  • Database migrator (aka check if it has the right columns and wipe otherwise)
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated CHANGELOG.md
  • I have made corresponding changes to user documentation for new features
  • I have made corresponding changes to library documentation for API changes
  • I have added preferences to enable/disable this (blocked by Autoimport autodetection #516)

@lieryan
Copy link
Member

lieryan commented Nov 3, 2022

Hi bagel, thank you for the contribution.

Since this database is actually really just a "cache"/"index" of the files on disk, I think we don't really need a full blown migration. So, rather than checking whether the database columns matches, we can just have a table to store metadata for the database file, e.g. the version of rope that created the database. If the rope version doesn't match, then just wipe the tables.

For convenience during development of rope itself, where version number may not reflect actual on-disk changes, we can just wipe the database when the hashes of the files in rope/contrib/autoimport directory changes, but I think even a manual wipe with rm -r .ropeproject would have been fine.

@bagel897
Copy link
Contributor Author

bagel897 commented Nov 3, 2022

Good point - the tests are in memory so they should work but I'll look into the version table

@lieryan
Copy link
Member

lieryan commented Nov 15, 2022

Hi Bagel, thanks for this, the changes for collecting documentation looks reasonable to me. However, I'm not quite seeing the reason why it is necessary to index the docstrings, maybe you can help me understand here.

Autocompletions need to index names because it needs fast search of the python names as the user types, but once you get the possible matches, wouldn't it be possible to just retrieve the docstring when asked by the editor in real time rather than preemptively returning all docstrings? For example, LSP >= 3.16.0 allows lazy resolution of an autocompleted name's documentation.

Resolving in real-time would allow for more complex resolution strategies as well that wouldn't really be easy during indexing time, for example, if a name is aliased or re-exported by another module, or you can retrieve the documentation for global object instances by inferring the type of the object and finding the docstring of its class.

@bagel897
Copy link
Contributor Author

That is a valid point - this can't get more than like 40% of docstrings so an import based solution would be better for these. On the other hand, I think the increased DB size and time isn't too bad, so it may be worth it to add it here as a baseline and let other projects build atop it (perhaps they could import if its missing/blank?). Also importing has the arbitrary code execution issue while this is AST based.

@bagel897 bagel897 force-pushed the autoimport_docstring branch from eb8e5fb to 6476478 Compare January 10, 2024 17:24
commit c4b7ada05a63c92c94e17cd6383c90284a16b820
Author: bagel897 <[email protected]>
Date:   Wed Jan 10 11:16:51 2024 -0600

    reformat

commit eb8e5fb
Author: bagel897 <[email protected]>
Date:   Mon Nov 7 23:17:15 2022 -0600

    Clean docs, add async support

commit a07151a
Merge: 1f0556c 764bbe1
Author: Bagel <[email protected]>
Date:   Thu Nov 3 20:46:31 2022 -0500

    Merge branch 'master' into autoimport_docstring

commit 1f0556c
Author: bagel897 <[email protected]>
Date:   Thu Nov 3 01:52:32 2022 -0500

    Expose the modname

commit 3788d54
Author: bagel897 <[email protected]>
Date:   Thu Nov 3 00:52:52 2022 -0500

    initial docstring support
@bagel897 bagel897 force-pushed the autoimport_docstring branch from 8776c54 to 3099c2a Compare January 10, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants