search

onsails / lspkind.nvim

vscode-like pictograms for neovim lsp completion items
MIT License
1.47k stars 36 forks source link

Refactor plugin #44

Closed AndreAugustoDev closed 2 years ago

AndreAugustoDev commented 2 years ago

I refactored the plugin for my needs, without removing anything relevant I'm submitting this PR so my branch is known, and if the changes I made were helpful, merge into the main branch I also changed the name of the plugin, in place of the dash I put a dot, in order to follow the nomenclature that I see in other plugins (lspkind-nvim > lspkind.nvim) If necessary I can open an issue about the name if you really want to change it

onsails commented 2 years ago

Appreciate it. I was literally learning lua while initially writing this plugin, so refactor should have been happen some time :) Thanks for doing it. Re lspkind-nvim -> lspkind.nvim – I was also thinking about that so I accept it as well.

stevearc commented 2 years ago

I refactored the plugin for my needs, without removing anything relevant

Strongly disagree. The public API has completely changed, with no backwards-compatible shim, no deprecation message, and not even any documentation on how to migrate. This is hitting individual users (see linked dotfiles commit above), but also completely removes the public API I added in #26 to allow other plugins to rely on lspkind-nvim as a unified source of symbols.

I would strongly recommend reverting the PR until there is a backwards-compatible shim. If you absolutely must break the public API, at least announce the changes (common practice is to create a pinned issue called "Breaking Changes" that people can subscribe to) and wait for a bit.

I'm happy to put up another PR cleaning this up, but I won't be able to get to it for at least a few days.

wookayin commented 2 years ago

There are so many breaking changes contained in this PR written by a random person (see #45).

The author/mantainer should have managed such a breaking change carefully and test extensively before merging. Neither existed any documentation. This should not have been merged.

AndreAugustoDev commented 2 years ago

There are so many breaking changes contained in this PR written by a random person (see #45).

The author/mantainer should have managed such a breaking change carefully and test extensively before merging. Neither existed any documentation. This should not have been merged.

Sorry, I tried to make the plugin code better, but I didn't pay attention to some important points. I fixed and documented the breaks in a new commit. I would like to check the refactoring

wookayin commented 2 years ago

Thank you and I can understand and appreciate that you are helping improve the project. People can review and help to make sure we don't accidentally break things, and I would be happy to provide help as well.