Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make omnifunc() complete keywords based on LSP triggerCharacters #418

Merged
merged 5 commits into from
Dec 1, 2023
Merged

Make omnifunc() complete keywords based on LSP triggerCharacters #418

merged 5 commits into from
Dec 1, 2023

Conversation

girishji
Copy link
Contributor

Fixed 2 bugs:

  • add triggerCharacters to lsp list for omniComplete option
  • process triggerCharacters based keywords in omnifunc()

M autoload/lsp/completion.vim
M autoload/lsp/lspserver.vim

Fixed 2 bugs:
- add triggerCharacters to lsp list for omniComplete option
- process triggerCharacters based keywords in omnifunc()

M  autoload/lsp/completion.vim
M  autoload/lsp/lspserver.vim
@girishji
Copy link
Contributor Author

girishji commented Nov 29, 2023

Unit tests are failing in main branch also (without my changes). Can someone please look into this?
I ran a few tests by hand and output is as expected. Looks like test framework has problem.

- fixed omnifunc related tests
- diag related tests are still failing

M  autoload/lsp/completion.vim
M  test/clangd_tests.vim
@Shane-XB-Qian
Copy link
Contributor

  1. you may need to explain more? what your change was trying to fix, and omni-compl was truly different like autocompl, but i donot know if it was really wrong? or your change was correct?
  2. there were still some test failed.

@girishji
Copy link
Contributor Author

girishji commented Dec 1, 2023

  1. you may need to explain more? what your change was trying to fix, and omni-compl was truly different like autocompl, but i donot know if it was really wrong? or your change was correct?
  2. there were still some test failed.

Some LSP servers complete after non-keyword characters like ., >, [, etc. This was already handled in 24/7 completion but ignored in omni-completion. My changes bring omnifunc() to par.

Diag related tests in clangd-tests are failing in main branch also. Not related to my changes.

test/clangd_tests.vim Outdated Show resolved Hide resolved
test/clangd_tests.vim Outdated Show resolved Hide resolved
M  autoload/lsp/completion.vim
M  test/clangd_tests.vim
M  autoload/lsp/completion.vim
Comment on lines -117 to 119
if opt.lspOptions.autoComplete
if opt.lspOptions.autoComplete || opt.lspOptions.omniComplete
lspserver.completionTriggerChars =
caps.completionProvider->get('triggerCharacters', [])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps just remove this 'if' which to do such assignment always if it had such cap,
besides 'omni complete' seems not only depended on 'omniComplete' this option only.

@Shane-XB-Qian
Copy link
Contributor

Some LSP servers complete after non-keyword characters like ., >, [, etc. This was already handled in 24/7 completion but ignored in omni-completion. My changes bring omnifunc() to par.

or actually i tried some, seems even no your changes still can get omni-compl items after e.g '.'

@yegappan yegappan merged commit cb905c8 into yegappan:main Dec 1, 2023
1 check passed
@Shane-XB-Qian
Copy link
Contributor

@yegappan merged? do you really get it was an improvement? please check above my comment, it seems it worked even no this changes, though that assignment perhaps should do it which regardless which kind compl.

@yegappan
Copy link
Owner

yegappan commented Dec 1, 2023

This is an improvement as the code for determining the trigger kind and trigger character are now the same between auto completion and omni completion. Before this change, omni completion always passed an empty string as the trigger character.

@girishji
Copy link
Contributor Author

girishji commented Dec 1, 2023

@yegappan merged? do you really get it was an improvement? please check above my comment, it seems it worked even no this changes, though that assignment perhaps should do it which regardless which kind compl.

If you do not pass trigger char to LSP server you'll not get appropriate completion. You'll get generic completion that it would send even without any keyword before cursor. You can verify this with 'pylsp' server. 'clangd' behaves differently, as it sends appropriate completion even without trigger char. Since unit tests only use clangd this problem was not exposed.

@Shane-XB-Qian
Copy link
Contributor

Shane-XB-Qian commented Dec 1, 2023 via email

@Shane-XB-Qian
Copy link
Contributor

Shane-XB-Qian commented Dec 1, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants