Skip to content

plugins/lspkind: migrate to mkNeovimPlugin #3563

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

saygo-png
Copy link
Contributor

@saygo-png saygo-png commented Jul 16, 2025

Relevant tracking issue: #2638

Copy link
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

This is a difficult plugin to modernize, as it has several bespoke options and seems to be "set up" in different ways depending on the use case.

I think more of the hard-coded logic in nixvim can be dropped or deprecated, in favour of users directly interacting with freeform settings.

I'm fine with still having extra options that controls which method of setting up is used.

Alternatively, we could have different freeform settings options; one person setup method?

@saygo-png saygo-png force-pushed the lspkind-mkNeovimPlugin branch 2 times, most recently from 64de6e3 to ea34af5 Compare July 17, 2025 18:31
@saygo-png
Copy link
Contributor Author

Sorry, this diff got quite noisy and i forgot to update the test so there are two pushes.

  • I removed the extra options you mentioned before, since they can just be defined in settings.
  • Added an assertion to make sure the user only has the cmp integration enabled when cmp is also enabled, since thats the only case where enabling this option makes sense
  • Thanks to the assertion doCmp is no longer needed and i just directly use cfg.cmp.enable
  • I added another optional string so that no config gets added to cmp unless the integration is explicitly enabled

Alternatively, we could have different freeform settings options; one person setup method?

I don't understand what you mean by this.

@saygo-png saygo-png force-pushed the lspkind-mkNeovimPlugin branch from ea34af5 to 2f777cc Compare July 17, 2025 18:41
@saygo-png
Copy link
Contributor Author

The test is failing now because the "empty" attribute set doesn't enable the cmp plugin.
I don't really understand what the different attribute sets in the test files are meant to be, so i'm not sure if i should add the cmp plugin to the "empty" one or disable the lspkind cmp integration instead.

These test failures make me doubt the addition of this assertion. It will break existing configs, since cmp integration is enabled by default. Should i just remove the assert, or maybe switch cmp integration to false by default?

@saygo-png saygo-png force-pushed the lspkind-mkNeovimPlugin branch from 2f777cc to 3c3f420 Compare July 17, 2025 20:09
@saygo-png saygo-png force-pushed the lspkind-mkNeovimPlugin branch from 3c3f420 to 91ed04f Compare July 26, 2025 10:47
@saygo-png
Copy link
Contributor Author

Rebase

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.

2 participants