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

Added support for google-style docstrings #963

Closed
wants to merge 2 commits into from

Conversation

Erotemic
Copy link

This a much reduced version of PR #796 with the goal of adding support for using Google style docstrings for paramter / return type inference.

Thanks to @davidhalter for the comments on the original PR. Also, thanks so much to the authors of #868 for working in the fixes I made when I originally tried to implement this functionality. Having those changes in made this a much easier job the second time around. That being said, this PR is much more self-contained.

The main changes here are:

  • A new module jedi/evaluate/docscape_google.py for parsing/scraping google-style docstrings
  • I called those functions in the appropriate places in jedi/evaluate/docstrings.py
  • I added tests

Looking at the diff, I see my editor took out a bunch of trailing whitespace. If you want me to undo that I can.

@Erotemic
Copy link
Author

All CI-tests are now passing on this branch (except for pypy, which I believe is failing on master as well).

@Erotemic
Copy link
Author

@davidhalter If I fix the conflicts can this PR get merged? PyTorch currently uses Google style docstrings and it would be very useful for jedi to work with that library.

@Erotemic Erotemic force-pushed the parse_google_docstr2 branch from 9627274 to 748ac18 Compare March 23, 2018 02:38
@Erotemic
Copy link
Author

Conflicts have been fixed.

@ArkadiuszNiemiec
Copy link

Is this PR dead or was is implemented somewhere else?

@Erotemic
Copy link
Author

I still want the feature, but I haven't tried to get it merged in awhile.

If @davidhalter is interested in merging the feature, I could probably spruce this up to be compatible with jedi-master. The CI tests pass, not sure how important -0.009% of coverage is.

@davidhalter
Copy link
Owner

Yeah sorry. It's really my bad that I never got back to it. There's just too many things that are keeping me busy.

This branch is well written and everything, so no offense @Erotemic, I feel like you tackled this problem well and found a good solution.

All in all I won't merge for a couple of reasons:

  1. This is a lot of code - even just to review it.
  2. A lof of code means a lot of maintenance. Every ten lines less are ten lines less I have to rewrite if Jedi fundamentally changes again (which it did a lot in recent years).
  3. It's not a feature that is massively used, only very few projects (mostly commercial) are using it, you can also see that this PR has only a very low amount of 👍 compared to other features.
  4. Jedi supports type annotations. It's the future for Python, use it instead of all these stupid docstrings :)

All in all there's just not much that speaks for merging it. I struggle to find the upsides, except for the well written code. I guess that's also why I couldn't bring myself to close it sooner.

Please keep in mind I do this all in my spare time and these additional 600 lines cost me 10+ hours a year (just for maintenance and bug reports). If you sum that up for all the feature/pull requests I get, I just have to cut my losses and decide what's actually worth it.

So sorry for not being clearer a few years ago.

@davidhalter davidhalter closed this May 2, 2019
@Erotemic
Copy link
Author

Erotemic commented May 7, 2019

While I'm disappointed, I understand the reality and cost of maintaining open source projects on your own time. While I want to respond to some points, I ultimately respect your decision.

This is a lot of code - even just to review it.

There is certainly a lot of logic (300 lines) in docscrape_google.py, and I can understand that it's difficult to review.

However, if you don't count that file, there are only 49 lines of code that modify Jedi (23 of which are doctests and documentation). Those 26 lines of logic simply follow existing patterns used to support the other Epydoc and Numpydoc docstring styles.

The remaining 202 lines of code are for tests, which again mirror the existing tests for other docstring styles.

It's not a feature that is massively used, only very few projects (mostly commercial) are using it

I'm not sure where you're getting that mostly commercial observation. Its certainly less used than other styles, but it has been catching on. Large open-source projects like pytorch and many academic deep-learning research repos use google style docstrings.

Jedi supports type annotations. It's the future for Python, use it instead of all these stupid docstrings :)

I wish I could. Unfortunately, myself and many others are stuck writing code that has to be 2/3 compatible for the foreseeable future. Even so, type annotations have are considerably less flexible than docstring types because they need to be defined in-code. Doing anything semi-complex with them can slow down the startup time of an application, and that's a big downside. While type annotations are great, they aren't a perfect solution.


As a closing thought, would you be more inclined to consider merging if I separated out the docscrape_google.py into its own pip-installable external package (much like numpy uses the numpydoc.docscrape module?). In fact, there is already a pip installable version of this file in the xdoctest package, but because its useful in its own right, it makes sense to make it its own package. It could be an optional dependency that only fools like me would install to make their development lives a bit easier.

@Erotemic
Copy link
Author

Erotemic commented May 7, 2019

While this PR hasn't received as many emojis as other PRs, five emojis is still a non-zero amount of emojis. For those people, I want to note that I'm going to look into an alternative solution to this problem. Because the actual logic required to implement this capability into jedi is relatively small, (23 lines), I can probably create some logic that monkey-patches the jedi/evaluate/docstrings.py module to inject this functionality. I'll link to that here once its done.

@ArkadiuszNiemiec
Copy link

ArkadiuszNiemiec commented May 7, 2019

@Erotemic I just want to say that I also have to maintain 2/3 compatible code and I am sure there's a lot of devs like us. I don't know if I can help you with development but please update here and ask if you will need a tester or any other help.

Edit: I have just added an emoji reaction to the PR 🥇

@Erotemic
Copy link
Author

Erotemic commented May 7, 2019

@ArkadiuszNiemiec

I've made a new PR for my vimtk library, which adds a function that monkey patches jedi with the contents of this PR. If you use vim, then once I accept that vimtk-PR and release the new version (in a few days), you can install vimtk and then simply add the following lines to your vimrc to explicitly enable the monkey patch:

if has('python3')
    command! -nargs=1 Python2or3 python3 <args>
elseif has('python')
    command! -nargs=1 Python2or3 python <args>
else
    echo "Error: Requires Vim compiled with +python or +python3"
    finish
endif
Python2or3 << EOF
from vimtk import jedi_monkeypatch
jedi_monkeypatch.apply_monkey_patch_jedi()
EOF

If you don't use vim, or don't want to install vimtk, then you may be able to follow the monkey patch logic in this function:

https://github.com/Erotemic/vimtk/pull/2/files#diff-17489446620e53eb9a11adc3185cf8a1

Its fairly simple. It checks the jedi version to ensure the monkey patch still works, and then it replaces the relevant jedi functions that handle docstrings with modified ones that also handle google-style docstrings. I left in debugging statements as I've given up on doing things the clean way for this feature.

@davidhalter
Copy link
Owner

@Erotemic The problem is jedi/evaluate/docscrape_google.py, not the other LOC that modify Jedi a tiny bit. I don't care about that. I care about the 300 lines that I have to maintain. It's also not like I wouldn't reconsider in 2 years or something like that or maybe you guys could just use the plugin system once it exists #626, but that's just years away. It's more honest to close this now and maybe allow it back in at a later point in time.

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.

3 participants