PR: Fix completions bug in Kite introduced by PR 10605

[metalogical]

Description of Changes

Fixes exception raised due to #10605 (described here)

How can we make sure we don't regress again? Maybe we need fixturized test cases with sample completions dicts that we return to the completions logic and ensure it works properly with textEdits. Thoughts?

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @metalogical

[metalogical]

I'm currently trying to add a test that mocks the completions response in various ways.

[ccordoba12]

I'm currently trying to add a test that mocks the completions response in various ways.

That sounds very nice, thanks! However, we'd prefer integration tests, so if you could add tests that call Kite directly, that would be much better.

[pep8speaks]

Hello @metalogical! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-07 17:58:33 UTC

[metalogical]

@ccordoba12 I added a unit test (and fixtures) that allow mocking the completions responses, and verify that a simple dict key completion example with textEdit works correctly.

I agree that integration tests would be nice, but I'm not sure what the best approach to get that working is. We have a kited_test binary that we use in CI for other editor plugins, which disables some excess logic (such as authentication, etc), and also exposes some additional endpoints useful for testing.

However, we'd need to set up a way to retrieve the this binary from the web or somewhere else. I think it's worth investing in (and we'd be willing to spend resources here whenever we have them), but would prefer to tackle this as a separate issue.

[ccordoba12]

Why the . at end of my_dict is necessary here?

[metalogical]

The reason I use this test case is because this is a type of completion that Kite returns (it allows users to quickly access dict keys with a .), and it's a case where the completion ["dict-key"] replacing the . can't be represented with just insertText.

[ccordoba12]

Ok, I didn't know that and it's pretty cool!

[metalogical]

Thanks for the look-over @ccordoba12

I addressed your comments.

[andfoy]

Just for the record, a contributor is migrating pyls completions to use textEdit: palantir/python-language-server#690

[metalogical]

@andfoy interesting

Note that currently Spyder's support for textEdit completions is very limited: if the user keeps typing, it filters out all such completions, due to the fact that the replacement range may be out-of-date. I didn't want to add a lot of complexity, so Spyder just depends on completions being re-requested in these cases.

If pyls starts returning those, it may be wise to invest further in expanding support for those completions. Especially since re-requesting completions with pyls may be slow.

EDIT: on further investigation, that PR doesn't remove insertText, so Spyder would just default to that I think.

[ccordoba12]

Looks good to me now. Thanks @metalogical!