Invoking completion after ( replaces the word that comes before

[jqno]

Steps to reproduce

1. Check out https://github.com/jqno/EnsimeVimCompletionIssue
2. Open CompletionTest.scala
3. Write print and invoke completion; choose println.
4. Write (
5. Write he and invoke completion

Expected behaviour

The line is expanded to println(hello)

Actual behaviour

The line is expanded to hello)

More info

I tried to fix this bug and offer a PR, but unfortunately I couldn't figure it out. I have noticed that fun_en_complete_func is invoked with a base parameter of println(he, where I would have expected just he. However, it seems to come directly from VIM that way, so I'm not sure what the intended approach should be:

• Try to get VIM to send he to ensime-vim instead of println(he
• Adapt ensime-vim to return println(hello instead of just hello
• Another approach entirely?

It seems to make sense to combine this with #271. I'd like to take a stab at that too, if you can give me some pointers :).

[ktonga]

I was able to reproduce it. It also happens invoking completion after [.

I can take a look, or help you with anything you need if you already started working on it.

Thank you for reporting.

[jqno]

Hi! I haven't started yet, but I'm certainly willing to give it a shot if you can tell me a good place to start.

[jvican]

Hey @jqno, thanks for taking a look into this!

For solving this, I recommend you to have a look at the vim and neovim docs: :h omnifunc and :h compl-omni. From there, you'll find interesting links that will shed some light into how omnicompletion works.

Have also a look at this function, which is the one implementing the client logic for the completion. The logic is a little bit complex because there are two handlers. Vim and neovim also implement omnifunc with a slight difference, so both need to be checked for correctness.

[jqno]

Hi @jvican,

I should have known that the answers I was looking for, were to be found in Vim's help system :). Thanks for pointing out the relevant sections to me, after reading up a little it was easy to fix this bug. See PR #360.

I'd like to add some tests as well, but I wasn't sure if I should write them in Cucumber of 'plain' Python.

[ktonga]

Re: testing,

For auto-completion it should be really sweet to have end-to-end tests since it's a functionality that interacts a lot with the editor (the Vim instance), in this case the best is to write some BDD tests using Lettuce (already integrated in the project), but before putting any more effort on it, it should be nice to have #312 merged.

On the other hand, to verify the correctness of methods like detect_row_column_start it should be enough to just add a few unit tests using pytest since thanks to some recent refactors introduced by @ches it is much easier to mock some of the Vim behaviour.

[jqno]

Thanks for the info. I think I'll look into making a few pytests for the detect_row_column_start behavior first, and look into making a BDD test once the Docker work is sorted out.

[ches]

It appears the completion logic implementation is all still inside EnsimeClient and somewhat entangled in its state, so it's difficult to unit test as it currently stands. ☹️

This seems like a good candidate to factor out parts that aren't server request/response to Editor as an API function that is easy to unit test—EnsimeClient's job to do server communication, Editor's job to interpret Vim specifics like the omnifunc API.

That might be clunky to achieve right now though with our polling thread-based implementation—what comes to mind for me is to have the Editor completion API accept cursor pos plus a coroutine or future that will eventually yield the suggestions. That would be straightforward on a Python 3 asyncio basis I'm eventually hoping we have… But if anyone sees a decent way to design this separation of concerns with our current arch, please take a crack at it by all means!

[jqno]

Being new to both Python and VIM plugins, I don't feel qualified to say anything about the architecture. I have some experience writing tests for difficult to test code though, so I'll try to write some tests next week or so. Then I'll let you decide whether it's the right approach or not.