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

Suggestion to simplify source.provider config #144

Closed
chrisgrieser opened this issue Oct 19, 2024 · 2 comments
Closed

Suggestion to simplify source.provider config #144

chrisgrieser opened this issue Oct 19, 2024 · 2 comments

Comments

@chrisgrieser
Copy link
Contributor

chrisgrieser commented Oct 19, 2024

The problem

As some issues (e.g., #132) or discussions on reddit show, the current way to configure sources appears to be confusing to some users. I think the problem derives from the fact that sources.providers is a list and not a dictionary.

Using lists of non-primitives is somewhat counterintuitive for many people, since the way vim.tbl_deep_extend() works, you need to copypaste the whole list if you want to adjust one property in one of the list elements. This leads to confusions, as can be seen in #132. Using lists of non-primitives is a pretty rare occurrence in nvim plugins to begin with, probably due to this problem.

Other than being less intuitive, the need to specify the full list unnecessarily creates the need to copy a lot of default configs, even if you intend to only change one detail. Furthermore, exposing "blink.cmp.sources.lsp" to the user feels unnecessary to me, just referring to a source via its name "LSP" should be enough?

Suggested change

I suggest using a dict to configure the individual sources, something like this:

sources = {
	enabled = { "LSP", "Snippets", "Path", "Buffer" },
	config = {
		LSP = {
			keyword_length = 0,
		},
		Snippets = {
			score_offset = -3,
			opts = {}
		},
		Path = {
			score_offset = 3,
			opts = {},
		},
		Buffer = {
			fallback_for = { "LSP" },
		}
	}
},

Comparison of the current config and the suggested change

If a user just wants to specify the path to their snippet folder, but not change anything else regarding blink.cmp's default config, their config currently will have to look like this:

sources = {
	providers = {
		{ "blink.cmp.sources.lsp", name = "LSP" },
		{ "blink.cmp.sources.path", name = "Path", score_offset = 3 },
		{
			"blink.cmp.sources.snippets",
			name = "Snippets",
			score_offset = -3,
			opts = {
				search_paths = { "/my/snippets" },
			},
		},
		{ "blink.cmp.sources.buffer", name = "Buffer", fallback_for = { "LSP" } },
	}
},

With the suggested change, they only need to do this:

sources = {
	config = {
		Snippets = {
			opts = {
				search_paths = { "/my/snippets" },
			},
		},
	},
},
@Saghen
Copy link
Owner

Saghen commented Oct 20, 2024

Yeah, totally agree with you, I've got a few commits locally implementing this:

sources = {
  --- @type string[] | fun(ctx: blink.cmp.Context): string[]
  completion = { 'lsp', 'path', 'buffer', 'snippets' } 
  cmdline = { 'commands', 'path' }
  search = { 'buffer' },

  providers = {
    lsp = {
      --- @type string
      name = 'LSP',
      --- @type string
      module = 'blink.cmp.sources.lsp',

      --- @type fun(ctx: blink.cmp.Context)
      enabled = true,
      --- @type table
      opts = {}

      --- @type fun(ctx: blink.cmp.Context, items: blink.cmp.Item[]): blink.cmp.Item[]
      transform_items = nil,
      --- @type fun(ctx: blink.cmp.Context, items: blink.cmp.Item[]): boolean
      should_show_items = nil,
      --- @type number | fun(ctx: blink.cmp.Context, items: blink.cmp.Item[]): boolean
      max_items = nil,

      --- @type number
      min_keyword_length = nil,
      --- @type string[] | fun(ctx: blink.cmp.Context, enabled_sources: string[]): string[]
      fallback_for = {},
      --- @type number | fun(ctx: blink.cmp.Context, enabled_sources: string[]): number
      score_offset = 0,

      -- override any builtin function of the source
      override = { ... },
    },
    path = { ... },
    buffer = { ... },
    snippets = { ... }
  },
}

Not settled on this though so feedback is welcome

@lopi-py
Copy link
Contributor

lopi-py commented Oct 21, 2024

just giving some ideas

sources_list = {
  'lsp',
  'path',
  'snippets',
  'buffer',
},
sources = {
  snippets = {
    opts = {
      search_paths = { '/my/snippets' },
    },
  },
},

sources_list may be split into

-- is "auto" in "autocompletion" a bit too much?
sources_completion = { "lsp", "path", "snippet", "buffer" },
sources_cmdline = {
  [":"] = { "cmdline" },
  ["/"] = { "buffer" },
  ["?"] = { "buffer" }, 
},
sources_by_ft = {
  gitcommit = { "git" },
}

blink will search for sources in blink.cmp.sources.[source_name], so each source must have to define their own name (imo this feels more right), e.g.

local source = {}
source.name = "LSP"

function source.new()
  return setmetatable({}, { __index = source })
end

@Saghen Saghen closed this as completed in 7fea65c Oct 24, 2024
mikavilpas added a commit to mikavilpas/blink.cmp that referenced this issue Nov 26, 2024
No references to this field are left, so it looks like it was left
behind from the issue Saghen#144

and closed by the commit in that issue,
Saghen@7fea65c
Saghen pushed a commit that referenced this issue Nov 26, 2024
No references to this field are left, so it looks like it was left
behind from the issue #144

and closed by the commit in that issue,
7fea65c
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

No branches or pull requests

3 participants