search

vheon / JediHTTP

Simple http wrapper around jedi
Apache License 2.0
40 stars 9 forks source link

Add support for jedi's call_signatures feature #6

Closed ptrv closed 6 years ago

ptrv commented 8 years ago

Review on Reviewable

vheon commented 8 years ago

Hi, Thanks for the PR! I was already experimenting on this in a local branch and wrote almost the same code but I want to hold off this API until I see how we want to handle it in ycmd which is still unclear how we want to do it.

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

_Comments from the review on Reviewable.io_

ptrv commented 8 years ago

Ok, I see. I have already an experimental branch in ycmd to implement this https://github.com/Valloric/ycmd/compare/master...ptrv:python-get-call-signatures

My idea was to be able to get the function arguments as soon as you typed the opening parentheses and get the function arguments and the index of the current argument.

vheon commented 8 years ago

I have putted a couple of additional field but the idea is the same. What I'm worried about is how this is going to play with python named arguments. That is the main reason I didn't already pushed the API; from a little experiment if I have:

def foo( bar = 1, qux = None ):
  pass

if I ask for the call_signatures in a situation like foo( qux = after the = I get the this is in fact argument 1 (so the second one) but if I then continue foo( qux = None, then I still get that after the , I'm in parameter 1 which I don't know if we should consider it correct.

ptrv commented 8 years ago

Ahh, I know what you mean. Hmm, for me it seems to be more of a jedi issue.

lithammer commented 8 years ago

What's the status of this? Seems like it's mostly done. Would be pretty sweet to have (got a taste for it when I experimented with deoplete.nvim).

vheon commented 8 years ago

@renstrom honestly? I don't remember the status of this 😝 Lately I don't much time on hand but I will try to get back to this ASAP

ptrv commented 8 years ago

I think a good concept for integrating this feature into ycmd was missing. And also jedi had an issue with default parameters as @vheon mentioned in his second comment.

zzbot commented 7 years ago

:umbrella: The latest upstream changes (presumably #38) made this pull request unmergeable. Please resolve the merge conflicts.

micbou commented 6 years ago

This should be implemented directly in ycmd now that PR https://github.com/Valloric/ycmd/pull/1028 is merged.