tzachar / compe-tabnine

TabNine source for hrsh7th/nvim-compe
BSD 3-Clause "New" or "Revised" License
52 stars 3 forks source link

Apply suffix changes returned from TabNine #8

Closed freehaha closed 3 years ago

freehaha commented 3 years ago

Hi,

Not the best PR format since it's actually 2 parts:

  1. Applies old_suffix -> new_suffix replacement from the result of TabNine. This requires proper mapping of compe's compe#confirm() function to confirm a completion.
  2. update determine function to trigger as a character-triggered source, which will trigger the completion on symbols as well as letters (for example, it will trigger completion on both Array| as well as Array.|)

One thing I'm not entirely sure is the undocumented keyword_pattern_offset you could return along with the completion items

            keyword_pattern_offset = pos[2] - #old_prefix + 1;

this is necessary to update the proper completion starting point for compe to compute the final list to display (because we use 1 as the initial value when triggering the completion and TabNine calculates the common prefixes of its suggestions.)

I've been using this setup for a week and it is working well for me but suggestions are always welcome.

freehaha commented 3 years ago

As I understand you patch, there are two major changes and a minor one. First, the tweak to determine, which makes the source trigger on every character. I like it, and it would be really easy to merge. Can you submit this in a different pull request? Or I can just make this change myself if its easier.

It's totally fine with me if you want to cherry pick the commits for this part.

Last, you add support for replacing selected text with the new_prefix returned from TabNine. In general, this would be really nice to have, but it does not always work as expected in every case. Moreover, the way TabNine adds closing brackets by default is annoying to some. I think the correct approach here would be to let compe handle this in general as @hrsh7th is already working on this. I suggest we wait for him and see if we need to add specific support inside TabNine source.

Personally I think TabNine(TN) handles the suffix pretty well compare to any auto-pair plugins I have ever used in vim, but we can always make it an option for this completion source.

It will also make it easier to handle some suggestions because if you don't do this you'll need to handle situations where TN's suggestion contains parts of the text after the cursor:

      string.format("queries/%|")

in this case you might get the following suggestions from TN:

if we don't handle the suffix changes, we would need to compute and subtract the duplicated part by examining the new_suffix value otherwise the completion will make very little sense and the line will become (using the first item)

string.format("queries/%s/%s", lang, query_name)")

instead of the more sensible one:

string.format("queries/%s/%s", lang, query_name)

IMHO this will make it kinda messy and redundant when TN already does a pretty good job at it. I guess it can be an optional feature but it's gonna feel very clunky when you have to do manual post-completion edits when it really should have been done automatically for you and is the point of using TN.

edit: typos

tzachar commented 3 years ago

Ok, been using this PR for a while, and it feels better. Let me test it for a few more days and i'll merge it.

tzachar commented 3 years ago

Working gr8 for me. Merging. 10x for you contribution @freehaha