[Bugfix #808] Tooltip for nested call

[rgom]

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

[fabioz]

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

[rgom]

I will do that, of course. For the time being feel free to revert the change. I can see 3 test cases failing. At least two of them used the following test code: "xxx(%s)" whereas my code used "xxx(" form. Possibly that could lead to different code paths, but I'll have to debug that. Regards, Robert

…

On Mon, Jun 5, 2017 at 3:21 PM, Fabio Zadrozny ***@***.***> wrote: I applied it here and it seems like the tests from PythonCompletionCalltipsTest broke... can you check why those broke with your fix? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#203 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH2lBz3AFYQ_WMzvX3iMqEt1gs6huxrgks5sBAD0gaJpZM4NwAdk> .

[rgom]

Ok, here it comes: 1. My initial fix was incorrect - sorry for that. 2. I haven't initially tested (executed) whole org.python.pydev.editor.codecompletion.PythonCompletionWithoutBuiltinsTest 3. My newly added test was incorrect - tried to substitute string without %s with parameters, which led to incorrect pass. After some rework: 1. Code of getBeforeParentesisCall refactored 2. Whole PythonCompletionWithoutBuiltinsTest file passing. I couldn't set up full environment properly, so executing whole tests_completions resulted in 37 errors and 4 failures, but all of them are from incorrect setup as I see it. 3. I have changed the following code in the test case to: Document doc = new Document(s); prop.apply(doc); // TODO: code below not working - proposal not applied String expected = StringUtils.format(original, ""); assertEquals(expected, doc.get()); As I understand, it means that my change fixes tooltip (calltip?) only, leaving prop (prop.apply) unfixed. That's reflected in the test code, which is incorrectly passing in that part. 4. Everything has been forcibly pushed to the same branch, what would you like to do next (I'm little bit afraid to push it too early this time)? a. Create MR as-is b. Create MR after removing this not working test part from the end or changing the comment to be more accurate c. Wait to have prop.apply debugged and fixed somehow d. Others? Regards, Robert On Mon, Jun 5, 2017 at 4:16 PM, Robert Gomułka < [email protected]> wrote:

…

I will do that, of course. For the time being feel free to revert the change. I can see 3 test cases failing. At least two of them used the following test code: "xxx(%s)" whereas my code used "xxx(" form. Possibly that could lead to different code paths, but I'll have to debug that. Regards, Robert On Mon, Jun 5, 2017 at 3:21 PM, Fabio Zadrozny ***@***.***> wrote: > I applied it here and it seems like the tests from > PythonCompletionCalltipsTest broke... can you check why those broke with > your fix? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#203 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AH2lBz3AFYQ_WMzvX3iMqEt1gs6huxrgks5sBAD0gaJpZM4NwAdk> > . >

[fabioz]

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]

Oh, that's fine. The patch tried to address just that case and could be missing some global picture. If the risk of introducing it is too high, please disregard it. Regards, Robert

…

On Mon, Aug 28, 2017 at 7:52 PM, Fabio Zadrozny ***@***.***> wrote: 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). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#203 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH2lBzcTR_XyE98WXrYVl0wAjlpcNicQks5scv5xgaJpZM4NwAdk> .

[fabioz]

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