Skip to content

Separate signature from docstring on hover #623

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

Merged
merged 4 commits into from
Sep 11, 2019

Conversation

mpanarin
Copy link
Contributor

LSP supports MarkedString[] to specify render engine. We can definitely say that function signature should be rendered as python.
For the docstring, as we have no good way of parsing it to guess the engine, we can pass it as markdown.

@ccordoba12
Copy link
Contributor

@andfoy, please take a look at this one.

@andfoy
Copy link
Contributor

andfoy commented Jul 17, 2019

I think we should aim to support Markdown fully using MarkupContent, i.e., Finishing #348

@mpanarin
Copy link
Contributor Author

@andfoy Well, that pr implements rst2md converter. Which is wrong, as python docs come in different shape and sizes. And without some specific parsers/converters is there an actual point in that?
While here we basically say render signature as python, everything else as markdown (whatever there is).

@gatesn
Copy link
Contributor

gatesn commented Aug 20, 2019

I wonder if there's a set of "tell-tale" signs that a docstring is in markdown vs rst vs plain text. In such a way that if those signs aren't there, then it's likely it doesn't matter what format we treat it as.

e.g. hello is valid markdown, rst and plain text. Whereas ..<directive>: is clearly an RST doc.

@mpanarin mpanarin force-pushed the fix_hover branch 3 times, most recently from ecf3dbe to 635bf05 Compare August 21, 2019 10:59
@mpanarin
Copy link
Contributor Author

@gatesn I updated this pull requrest

In jedi 15.0 author added a proper way to get a signature of a function here which allows us to grab signature and docstring separately, formatting them accordingly.
Imo, these is reasonable enough change to make the dependency on jedi >= 0.15.0 as it can definitely be used in jedi_completion as well (I didn't check specific use, but I am sure it is there.) making code clearer.
If accepted, supersedes #635

About your comment on hover.contentFormat those are the types of MarkupKind - plaintext or markdown. So this should work fine as we follow markdown route.

For a tell-tale signs - this is a good thing for a research. But as I said earlier - it is a huge amount of work, and docstring at the moment are not highlighted properly. Which is a shame.

Copy link
Contributor

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

I left a minor comment, otherwise looks good to me.

@gatesn?

LSP supports `MarkedString[]` to specify render engine. We can definitely
say that function signature should be rendered as python. For the docstring,
as we have no good way of parsing it to guess the engine, we can pass it
as markdown.
@samiur
Copy link

samiur commented Sep 4, 2019

Would love to see this merged, as I'd like the Jedi requirements update

@mpanarin
Copy link
Contributor Author

mpanarin commented Sep 6, 2019

@ccordoba12 ping on this one )

@ccordoba12 ccordoba12 changed the title Fix hover Separate signature from docstring on hover Sep 11, 2019
@ccordoba12 ccordoba12 added this to the 0.29.0 milestone Sep 11, 2019
Copy link
Contributor

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks for your patience @mpanarin!

@ccordoba12 ccordoba12 merged commit c3cab77 into palantir:develop Sep 11, 2019
@maximbaz
Copy link

Hey guys, would you please cut off a new release targeted the latest python-jedi? It seems 0.29.0 milestone is complete anyway 😉

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.

6 participants