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

feat: add omnifunc completion source #1114

Merged
merged 5 commits into from
Feb 1, 2025
Merged

Conversation

FerretDetective
Copy link
Contributor

Closes #1103

@FerretDetective
Copy link
Contributor Author

A few thoughts:

  1. Should this source have any specific 'kind' for its completions?
  2. Should this to be opt-in, or enabled by default?
  3. Would it possibly make more sense to instead implement a general source for all complete-functions (completefunc, thesaurusfunc, and omnifunc)?

@milanglacier
Copy link
Contributor

milanglacier commented Jan 31, 2025

Should this to be opt-in, or enabled by default?

I think this should be opt-in. Omni-completion seems to be a synchronous interface? And neovim will automatically set lsp as omni-func by default when lsp is attched and omni-func is not set by the users. Probably don't want to have double-lsp.

@FerretDetective
Copy link
Contributor Author

Omni-completion seems to be a synchronous interface?

The complete-functions does seem to be synchronous, though it does mention using complete_add() and complete_check() for "when searching for matches takes some time." I don't know how much that matters for this application.

And neovim will automatically set lsp as omni-func by default when lsp is attched and omni-func is not set by the users. Probably don't want to have double-lsp.

By default I have v:lua.vim.lsp.omnifunc disabled because:

  1. Double-lsp
  2. It just doesn't work when enabled. For some reason invoking it causes it to hijack control and opens the <C-x><C-o> popup. This happened when I tested cmp-omni as well.

Either way, I don't disagree that it may make more sense for this to be opt-in.

@Saghen Saghen merged commit 59c3e21 into Saghen:main Feb 1, 2025
3 checks passed
@Saghen
Copy link
Owner

Saghen commented Feb 1, 2025

Thank you! Opt-in makes sense since the API is synchronous

Would it possibly make more sense to instead implement a general source for all complete-functions (completefunc, thesaurusfunc, and omnifunc)?

I'm open to renaming this omni source to make it clear that it's for all complete funcs. I likely won't have time to work on this but feel free to PR if that interests you

@FerretDetective
Copy link
Contributor Author

FerretDetective commented Feb 1, 2025

I think—at least for now—it might be best to leave it at omnifunc to avoid the complexity of having multiple complete-functions being used through a single blink source. Though, if it is requested I can have a look at submitting a PR for that.

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.

Upstream omnifunc completion source
3 participants