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

[Bugfix #808] Tooltip for nested call #203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rgom
Copy link
Contributor

@rgom rgom commented Jun 5, 2017

Fix tooltip showing for nested method call.
Calculate number of parentheses to skip instead of looking for the first
one.

@fabioz
Copy link
Owner

fabioz commented Jun 5, 2017

I applied it here and it seems like the tests from PythonCompletionCalltipsTest broke... can you check why those broke with your fix?

@rgom
Copy link
Contributor Author

rgom commented Jun 5, 2017 via email

Fix tooltip showing for nested method call.
Calculate number of parentheses to skip instead of looking for the first
one.
@rgom
Copy link
Contributor Author

rgom commented Jun 5, 2017 via email

@fabioz
Copy link
Owner

fabioz commented Aug 28, 2017

I checked it here... this is actually a pretty tricky area.

For instance, if I have the code (with the cursor at %s):

def foo(a, b):
    pass

def bar(c, d):
    pass

foo([bar(), %s])

then it should get a global completion (because it's inside a list and is not a part of the call -- would be the same inside a tuple, dict or set) or if there's something as

foo(bar(1, 'some(unbalanced(parens'), %s

it should note that it should skip the string.

I'm not sure the best approach here... ideally we'd discover the indentation and then compute the proper location and then interpret better inside which context we are (i.e.: method call, how many parameters are already passed to the current method call, list, tuple, etc).

Related to prop.apply, right now it's by design that we don't add text and just show a calltip (this happens in AbstractPyCodeCompletion.java (it changes the onApplyAction to just show the context info if there's already some parameter in the call) -- this could be changed, but the process of calculating things should be more robust before that's made the default and more support to know that just the second parameter should be added in the text because the first one is already there.

So, I'm not sure... the patch does fix the particular use case, but it still leaves a lot open to improvement (I have to check better if it doesn't go backward in some other currently supported area either).

@rgom
Copy link
Contributor Author

rgom commented Aug 28, 2017 via email

@fabioz
Copy link
Owner

fabioz commented Sep 5, 2017

I'll leave it open for now (I may take a better look at it in the future).

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