tzachar / cmp-tabnine

TabNine plugin for hrsh7th/nvim-cmp
MIT License
287 stars 28 forks source link

fix potential unmatch of ctx and response #34

Closed yioneko closed 2 years ago

yioneko commented 2 years ago

In the current implementation we memorize the context provided cmp as a queue and assign it to the corresponding response. There are two main problems here:

tzachar commented 2 years ago

Thanks for your work.

I like your approach. I tried something similar to begin with, which did not work. This approach is better.

I think that apart from some grammar fixes on the README, and pending some testing to see that it works, this can be merged.

However, I think a better solution would be to get some support from TabNine itself. See https://github.com/codota/TabNine/issues/498 . If it gets merged, the solution would be to keep the context and the callbacks together, use the suggested cookie for find the correct context and callback and use them. Should be more robust.

tzachar commented 2 years ago

Ok, you can see the answer to the request I opened on TabNine. Apparently, there is such an option of adding cookie to the request, which is then returned in the response.

Can you please rework the pull request to use the correlation_id field? I would suggest using a counter which is advanced every time a request is made, and save the contexts and callback in a table where the key is the counter at the time of invocation. Makes sense?

yioneko commented 2 years ago

Sure, I'm very looking forward to addressing the problem without the weird workaround. By the way, is it better to use the id field given in the ctx provided by cmp as correlation_id to keep sync with cmp? Not much sure about it.

tzachar commented 2 years ago

I think its safe. Look at https://github.com/hrsh7th/nvim-cmp/blob/ae708ef3a44dfb0cb42d1aa901b4c57c2de53aa3/lua/cmp/context.lua#L41 and https://github.com/hrsh7th/nvim-cmp/blob/ae708ef3a44dfb0cb42d1aa901b4c57c2de53aa3/lua/cmp/utils/misc.lua#L80 cmp increments the counter each time a new context is created.

Had a single review comment, otherwise it looks ready to merge.

yioneko commented 2 years ago

I couldn't see the comment, anything went wrong?

tzachar commented 2 years ago

Not wrong, a simple change. Its the last pending comment above

yioneko commented 2 years ago

Its the last pending comment above

I couldn't see the mentioned comment for changes in my browser 😂, what is the content of review comment?

tzachar commented 2 years ago

line 221: This should be lifted to row 215, as its not correct to return, rather you should continue.

yioneko commented 2 years ago

IDK what happens to github or my account but I still can't see the review comment left on where it should be 🤷‍♂️. After all, thanks for the suggestions and code review 😄.