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: pass kind to cmp #10

Merged
merged 1 commit into from
Apr 6, 2023
Merged

feat: pass kind to cmp #10

merged 1 commit into from
Apr 6, 2023

Conversation

3rd
Copy link
Contributor

@3rd 3rd commented Apr 5, 2023

This change would pass the symbol kind back to cmp.

It's very basic, and it converts the kinds from dot.case to CamelCase to overlap with at least some of the LSP kinds, and can also be used for filtering out things like Comments with entry_filter.

Could be done better by maintaining a tree-sitter <-> LSP kind mappings, but it's good enough for me, and maybe it helps someone else trying to work around all the completion items being Text.

Thanks for making the completion source, it's super fast and the workflow is awesome with LSP replacing the tree-sitter entries when it's done resolving.

cmp.mp4

@3rd
Copy link
Contributor Author

3rd commented Apr 5, 2023

Filtering example:

{
    sources = cmp.config.sources({
      {
        name = "treesitter",
        ---@diagnostic disable-next-line: unused-local
        entry_filter = function(entry, _ctx)
          local banned_kinds = {
            "Error",
            "Comment",
          }
          local kind = entry:get_completion_item().cmp.kind_text
          if vim.tbl_contains(banned_kinds, kind) then return false end
          return true
        end,
      },
    }),
}

Copy link
Owner

@ray-x ray-x left a comment

Choose a reason for hiding this comment

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

Thanks!

@ray-x ray-x merged commit 389eadd into ray-x:master Apr 6, 2023
@CharlesChiuGit
Copy link

CharlesChiuGit commented Apr 7, 2023

Hi, this dynamic kind_text causing my completion system to broke. I use lspkind's symbol_text setting. and this PR make it impossible to replace kind_text with icons. Is it possible to revert the PR and let it simmer a bit?

@3rd
Copy link
Contributor Author

3rd commented Apr 7, 2023

@CharlesChiuGit @ray-x
I think cmp-treesitter should provide cmp the known type of the nodes, and it would make more sense to add a default icon for symbol_text in lspkind.nvim, especially as they don't handle all the semantic tokens (macro, decorator, modifier, regexp).

Maybe we can remove the part that makes the tree-sitter identifier camelcase, I think that was some bad judgement from my side, and it would be better to give users the raw information and let them patch it with the format handler.

@CharlesChiuGit
Copy link

@3rd thanks for the quick reply! That would be a nice idea!
Do u have a list of all semantic tokens? If so I a can send a PR to lspkind.

@3rd
Copy link
Contributor Author

3rd commented Apr 7, 2023

@3rd I think I was wrong and it's not based on semantic tokens, but SymbolKind, it's missing at least Array, Object, Key from what I can see.
But it's ok, they probably don't need those, just a default icon option.

@CharlesChiuGit
Copy link

CharlesChiuGit commented Apr 7, 2023

hmmm, that's weird. I had add a list of icons to cmp.

And concatenate those tables and pass it to lspkind, if that's the case, I shouldn't have errors, no?

@3rd
Copy link
Contributor Author

3rd commented Apr 7, 2023

hmmm, that's weird. I had add a list of icons to cmp.

And concatenate those tables and pass it to lspkind, if that's the case, I shouldn't have errors, no?

Yep, should be ok, but you can't add an entry for each tree-sitter identifier, they're too many. Easiest way is to configure a default icon.
Best way would be to maintain a mapping of treesitter identifiers to lsp kinds, but that depends on language, etc.

@CharlesChiuGit
Copy link

hmmm, sounds reasonable. let me try if that's possible to add a default icon for this cmp source.

@CharlesChiuGit
Copy link

I think I misunderstood your meaning. You mean to add a default icon for cmp instead this certain source?

@3rd
Copy link
Contributor Author

3rd commented Apr 7, 2023

@CharlesChiuGit yep, in cmp, you can add it only to this source if you want:

      format = function(entry, vim_item)
        if entry.source.name == "treesitter" then
          ...
        end
      end

or you can use lspkind to get only the symbol, check if it's a symbol or a longer string, and if it's a longer string swap to a default symbol and append the kind.

but the best thing would be for lspkind to add a default symbol

@uga-rosa
Copy link
Contributor

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItemKind

nvim-cmp follows the type system of lsp; anything other than the 25 contained in CompletionItemKind should not be passed to kind field. If you want to use something unique, use the data field instead of the kind field.

@3rd
Copy link
Contributor Author

3rd commented Apr 24, 2023

@uga-rosa maybe the best way to do it is to let the users map the kinds themselves.
There's no reason why cmp shouldn't render the type information if it's available, but it doesn't come from a language server.
I'll make a PR for that to get rid of the case change and to not change the default behavior, while allowing tree-sitter types to not be all displayed as "Text".

@CharlesChiuGit
Copy link

CharlesChiuGit commented Apr 25, 2023

I'll make a PR for that to get rid of the case change and to not change the default behavior, while allowing tree-sitter types to not be all displayed as "Text".

Actually, I prefer the case change as default lol.

but it doesn't come from a language server.

Indeed, it's not from the LSPs, other cmp sources like https://github.com/tzachar/cmp-tabnine and https://github.com/Exafunction/codeium.vim also define their own kind_text. I don't see why the reason concatenate the ts types in data, won't that confuse more ppl? The format would be a mess.

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.

4 participants