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(cmp): enable the user to easily add new completion sources #3975

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ww-daniel-mora
Copy link

@ww-daniel-mora ww-daniel-mora commented Mar 24, 2023

Description

  • Consolidate lvim.builtin.cmp.formatting.source_names and lvim.builtin.cmp.formatting.duplicates into a new lvim.builtin.cmp.formatting.source_settings table
  • Update lvim.builtin.cmp.formatting.format function to use source_settings table to determine the menu name, icon, highlight group, and duplicate flag
  • Add menu names for crates, and lab.quick_data
  • These changes make it easy for users to add new completion sources by adding a new entry to the lvim.builtin.cmp.formatting.source_settings table
    summary of the change
  • This change will break any users who accessed lvim.builtin.cmp.formatting.source_names or lvim.builtin.cmp.formatting.duplicates directly

How Has This Been Tested?

Manual testing in config.lua

lvim.plugins = {
...
  {
    "jcdickinson/codeium.nvim",
    dependencies = { "hrsh7th/nvim-cmp" },
    config = function()
      require("codeium").setup({})
    end
  },
}
table.insert(lvim.builtin.cmp.sources, { name = "codeium" })
lvim.builtin.cmp.formatting.source_settings.codeium = {
  menu_name = "(Codeium)",
  icon = "",
  hl_group = "CmpItemKindTabnine",
}

…tion sources

* Consolidate lvim.builtin.cmp.formatting.source_names and lvim.builtin.cmp.formatting.duplicates into a new lvim.builtin.cmp.formatting.source_settings table
* Update lvim.builtin.cmp.formatting.format function to use source_settings table to determie the menu name, icon, highlight group, and duplicate flag
* Add menu names for crates, and lab.quick_data
* These changes make it easy for users to add new completion sources by adding a new entry to the lvim.builtin.cmp.formatting.source_settings table
Copy link
Collaborator

@kylo252 kylo252 left a comment

Choose a reason for hiding this comment

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

I'm not sure what the overall benefit here is, since you can already adjust/add/delete sources directly by accessing lvim.builtin.cmp.sources.

I suggest:

  • consolidating source_settings to the sources table instead, and avoid adding new options
  • deprecating any redundant and incompatible options that we already have (be sure not to break backwards compatibility)

@kylo252 kylo252 marked this pull request as draft April 7, 2023 12:21
@ww-daniel-mora
Copy link
Author

Hey @kylo252 thank you for taking a quick look.
As I mentioned in the description this change consolidates lvim.builtin.cmp.formatting.source_names lvim.builtin.cmp.formatting.duplicates into a new table lvim.builtin.cmp.formatting.source_settings

While you are correct that users can add new sources via the lvim.builtin.cmp.sources table, users cannot currently easily configure how the sources are displayed in the completion menu, as menu styling is set in the lvim.builtin.cmp.formatting.format function, which has special logic for styling copilot, tabnine, crates and a few other sources.

This change replaces the source specific logic with more generic logic that retrieves all the completion menu settings from the source_settings table for every source. This means that users can now if the user adds a new source via the lvim.builtin.cmp.sources table they can optionally add an entry to lvim.builtin.cmp.formatting.source_settings with the completion menu icon and completion menu name, among other settings.

If you take a look at the testing example in the PR it might help demonstrate what this PR enables.

As for backwards compatibility I did not find either of the deleted tables in any documentation so I believe they are internal. Please let me know if this is incorrect and they are considered part of the public API and I would be happy to deprecate the tables rather than remove them.

Thank you again. I'm looking forward to your response.

@ww-daniel-mora
Copy link
Author

@kylo252 I wanted to check back in before I make additional changes. I had some questions above and your guidance would be greatly appreciated.

  1. I want to make sure the benefit of the change is clear (easily add menu formatting for new sources)
  2. I wanted to clarify consolidation source_settings is already a consolidation of 2 tables. Should I collapse them into sources instead
  3. Can I delete source_names and duplicates since they are not documented or are they considered part of the public API and should therefore should be deprecated first.

@kylo252
Copy link
Collaborator

kylo252 commented Apr 14, 2023

While you are correct that users can add new sources via the lvim.builtin.cmp.sources table, users cannot currently easily configure how the sources are displayed in the completion menu, as menu styling is set in the lvim.builtin.cmp.formatting.format function, which has special logic for styling copilot, tabnine, crates and a few other sources.

is there no additional API that we can use from cmp's side?

cc: @abzcoding, @LostNeophyte

@ww-daniel-mora
Copy link
Author

Great question @kylo252 that I had not considered. The cmp way to control menu formatting is via the formatting.format function passed to setup (cmp wiki example).

Looking through cmp.lua the lvim.builtin.cmp object we are building up is actually what we pass directly to cmp setup

So this function formatting.format is the cmp way of setting the icons. Cmp has not documented a way of setting dev icons outside of setup.

There is an available workaround for this use case if the team choses not to accept the PR. LunarVim users can wrap and override the formatting function.

To satisfy my particular use case the following code in config works:

-- codeium setup
table.insert(lvim.builtin.cmp.sources, { name = "codeium" })
lvim.builtin.cmp.formatting.source_names.codeium = "(Codeium)"
local default_format = lvim.builtin.cmp.formatting.format
lvim.builtin.cmp.formatting.format = function(entry, vim_item)
  vim_item = default_format(entry, vim_item)
  if entry.source.name == "codeium" then
    vim_item.kind = ""
    vim_item.kind_hl_group = "CmpItemKindTabnine"
  end
  return vim_item
end

That being said I still believe the table approach in this PR is an improvement. Besides enabling new completion sources with dev icons it simplifies the formatting.format function replacing many if checks with a single table lookup.

@ww-daniel-mora
Copy link
Author

Hey @kylo252 just wanted to followup here. To succinctly reiterate my earlier responses & questions:

  1. No, there is no other cmp way to control menu formatting
  2. This is already a consolidation of two existing tables
  3. Do undocumented tables need to be deprecated before being deleted or can we just drop them?

If accepted, as a followup to this PR I would also like to contribute updates to the documentation on how to add new cmp sources with proper menu formatting.

Copy link
Collaborator

@kylo252 kylo252 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!

path = 1,
nvim_lsp = 0,
luasnip = 1,
source_settings = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about the naming here, and I'm still wondering if we're better off merging these into the original sources list, maybe under cmp.sources[foo].options

Copy link
Author

Choose a reason for hiding this comment

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

@kylo252 I actually really like because there are already a few source settings sprinkled into the sources list, see copilot for an example:

sources = {
      {
        name = "copilot",
        -- keyword_length = 0,
        max_item_count = 3,
        trigger_characters =
-- etc.

So consolidating the rest of the settings in there make sense. However, as you can see, the sources object is a list so we can't index in as you suggested cmp.sources[foo].options. We could change it to that format i.e.

sources = {
  copilot = { 
    -- options for copilot
  }
  other_source = {
   -- other options
  }

This would give us a single place to add sources and configure them in a way that I thinks makes sense, of course we would need to update everything that access the sources list to handle the new format.

lua/lvim/core/cmp.lua Outdated Show resolved Hide resolved
lua/lvim/core/cmp.lua Outdated Show resolved Hide resolved
lua/lvim/core/cmp.lua Outdated Show resolved Hide resolved
lua/lvim/core/cmp.lua Outdated Show resolved Hide resolved
Co-authored-by: kylo252 <59826753+kylo252@users.noreply.github.com>
Copy link
Author

@ww-daniel-mora ww-daniel-mora left a comment

Choose a reason for hiding this comment

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

Hey @kylo252 I've accepted all your suggestions, thank you, and I responded to your consolidation comment with some additional questions. Let me know what you think.

path = 1,
nvim_lsp = 0,
luasnip = 1,
source_settings = {
Copy link
Author

Choose a reason for hiding this comment

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

@kylo252 I actually really like because there are already a few source settings sprinkled into the sources list, see copilot for an example:

sources = {
      {
        name = "copilot",
        -- keyword_length = 0,
        max_item_count = 3,
        trigger_characters =
-- etc.

So consolidating the rest of the settings in there make sense. However, as you can see, the sources object is a list so we can't index in as you suggested cmp.sources[foo].options. We could change it to that format i.e.

sources = {
  copilot = { 
    -- options for copilot
  }
  other_source = {
   -- other options
  }

This would give us a single place to add sources and configure them in a way that I thinks makes sense, of course we would need to update everything that access the sources list to handle the new format.

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.

None yet

2 participants